clayborg added a comment.
Mostly good, but it seems weird to supply the cache line size when calling the
MemoryCache::Clear() function. We also don't seem to be catching updates to the
cache line size property and invalidating the cache when and only when the
property is modified, but we seem to be relying on calls to Clear() to do this
when we stop? It would be nice to clean this up before submitting just because
this change forces this now out of place looking setting to be passed along.
ProcessProperties::ProcessProperties() has a place where you can hook in and
call a function when the property changes:
ProcessProperties::ProcessProperties(lldb_private::Process *process)
: Properties(),
m_process(process) // Can be nullptr for global ProcessProperties
{
if (process == nullptr) {
// Global process properties, set them up one time
m_collection_sp =
std::make_shared<ProcessOptionValueProperties>(ConstString("process"));
m_collection_sp->Initialize(g_process_properties);
m_collection_sp->AppendProperty(
ConstString("thread"), ConstString("Settings specific to threads."),
true, Thread::GetGlobalProperties()->GetValueProperties());
} else {
m_collection_sp = std::make_shared<ProcessOptionValueProperties>(
Process::GetGlobalProperties().get());
m_collection_sp->SetValueChangedCallback(
ePropertyPythonOSPluginPath,
[this] { m_process->LoadOperatingSystemPlugin(true); });
}
}
So we could add:
m_collection_sp->SetValueChangedCallback(
ePropertyMemCacheLineSize,
[this] { m_process->UpdateCacheLineSize(); });
This function would then update the cache line size in the m_memory_cache
variable.
================
Comment at: lldb/source/Target/Memory.cpp:31-38
+void MemoryCache::Clear(uint64_t cache_line_size, bool clear_invalid_ranges) {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
m_L1_cache.clear();
m_L2_cache.clear();
if (clear_invalid_ranges)
m_invalid_ranges.Clear();
+ m_L2_cache_line_byte_size = cache_line_size;
----------------
Do we call clear when the cache line size changes? Maybe we should just have a
function that does this and only this?:
```
void MemoryCache::SetCacheLineSize(uint64_t size);
```
================
Comment at: lldb/source/Target/Process.cpp:614
m_image_tokens.clear();
- m_memory_cache.Clear();
+ m_memory_cache.Clear(GetMemoryCacheLineSize());
m_allocated_memory_cache.Clear();
----------------
Seems weird to be passing the cache line size to the clear method. Here we
clearly just want to clear out the cache and don't care about the cache line
size.
================
Comment at: lldb/source/Target/Process.cpp:1497
m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
- m_memory_cache.Clear();
+ m_memory_cache.Clear(GetMemoryCacheLineSize());
LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
----------------
Seems weird to be passing the cache line size to the clear method. Here we
clearly just want to clear out the cache and don't care about the cache line
size.
================
Comment at: lldb/source/Target/Process.cpp:5615
m_thread_list.DiscardThreadPlans();
- m_memory_cache.Clear(true);
+ m_memory_cache.Clear(GetMemoryCacheLineSize(), true);
DoDidExec();
----------------
Seems weird to be passing the cache line size to the clear method. Here we
clearly just want to clear out the cache and don't care about the cache line
size.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77790/new/
https://reviews.llvm.org/D77790
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
