Add memory dump providers for LocalStorageSqlite and SessionStorageSqlite [chromium/src : main]
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
Hi Steve, PTAL when you have a chance, thanks!
Related details
- Rahul Singh
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 7 comments![]( "Open in Gerrit")
Thanks for adding this!
"site_storage/sessionstorage/0x?/sqlite",Not your change, but why is session storage missing an entry for leveldb?
const bool uses_sqlite =
base::FeatureList::IsEnabled(kDomStorageSqlite) ||
(in_memory_ && base::FeatureList::IsEnabled(kDomStorageSqliteInMemory));Let's avoid duplicating this check, which will evolve over time. Instead, please re-use the `ShouldUseSqliteBackend()` helper function from dom_storage_database.cc. As part of this, you'll need to declare the function in the header while moving the function definition out of the anonymous namepsace.
auto* db_mad = pmd->CreateAllocatorDump(
context_name + (uses_sqlite ? "/sqlite" : "/leveldb"));
// Specifies that the current context is responsible for keeping memory alive.
int kImportance = 2;
pmd->AddOwnershipEdge(db_mad->guid(), global_dump->guid(), kImportance);Not your change, but I don't understand how this allocator dump links up to anything. It looks like LevelDB creates dumps with the name "leveldatabase/db_<ptr>". See DBTracker::MemoryDumpProvider::DumpAllDatabases().
As a sanity check, have you tried using chrome://tracing with the memory-infra category to compare the memory dumps for LevelDB and SQLite?
https://source.chromium.org/chromium/chromium/src/+/main:docs/memory-infra/README.md
CHECK(!meta_table_);nit: let's add a CHECK(!memory_dump_id_) to make sure we're not overwriting the dump id.
this, "DOMStorageBackingStore",Should we use separate names for local storage and session storage? Before the recent refactoring, local storage and session storage used the same database code.
std::string dump_name = base::StringPrintf("sqlite/db_0x%" PRIXPTR,Since we're creating this in feature code, should we add a more specific name? Following the conventions in local_storage_impl.cc, maybe something like "site_storage/localstorage/sqlite/db_0x%"?
Related details
- Rahul Singh
- Xiaohan Zhao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 6 comments![]( "Open in Gerrit")
Not your change, but why is session storage missing an entry for leveldb?
Good catch. I'm not sure why it was missing either, but I've added the leveldb entry for session storage for consistency.
const bool uses_sqlite =
base::FeatureList::IsEnabled(kDomStorageSqlite) ||
(in_memory_ && base::FeatureList::IsEnabled(kDomStorageSqliteInMemory));Let's avoid duplicating this check, which will evolve over time. Instead, please re-use the `ShouldUseSqliteBackend()` helper function from dom_storage_database.cc. As part of this, you'll need to declare the function in the header while moving the function definition out of the anonymous namepsace.
Done
auto* db_mad = pmd->CreateAllocatorDump(
context_name + (uses_sqlite ? "/sqlite" : "/leveldb"));
// Specifies that the current context is responsible for keeping memory alive.
int kImportance = 2;
pmd->AddOwnershipEdge(db_mad->guid(), global_dump->guid(), kImportance);Not your change, but I don't understand how this allocator dump links up to anything. It looks like LevelDB creates dumps with the name "leveldatabase/db_<ptr>". See DBTracker::MemoryDumpProvider::DumpAllDatabases().
As a sanity check, have you tried using chrome://tracing with the memory-infra category to compare the memory dumps for LevelDB and SQLite?
https://source.chromium.org/chromium/chromium/src/+/main:docs/memory-infra/README.md
Done
nit: let's add a CHECK(!memory_dump_id_) to make sure we're not overwriting the dump id.
Done
Should we use separate names for local storage and session storage? Before the recent refactoring, local storage and session storage used the same database code.
Done
std::string dump_name = base::StringPrintf("sqlite/db_0x%" PRIXPTR,Since we're creating this in feature code, should we add a more specific name? Following the conventions in local_storage_impl.cc, maybe something like "site_storage/localstorage/sqlite/db_0x%"?
- Rahul Singh
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |