IndexedDB: Add memory dump reporting for SQLite backing store [chromium/src : main]
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 5 comments![]( "Open in Gerrit")
Add memory dumping support for IndexedDB SQLite connections.Xiaohan ZhaoCan we briefly describe the name and structure of the memory dumps being added?
Done
Bug: 40253999Xiaohan Zhaocan you file a more specific bug for this please?
Done
"site_storage/index_db/db_0x?",Xiaohan Zhaonit: these should be indexed_db
Done
"site_storage/index_db/memenv_0x?",Abhishek Shanthkumarmemenv and db both seem unused? When did that become the case? Can we fix this?
Xiaohan ZhaoThey are both set by [TransactionalLevelDBDatabase::OnMemoryDump](https://source.chromium.org/chromium/chromium/src/+/main:components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.cc?q=site_storage%2Findex_db&ss=chromium).
Done
static_cast<sqlite::BackingStoreImpl*>(backing_store())Abhishek Shanthkumarall interaction with the backing store (except construction) should go through the backing store interface, meaning this cast should not be necessary.
Xiaohan ZhaoAdding to this, I believe we need to redefine what memory dumps IndexedDB makes in the SQLite world. Currently, with LevelDB:
1. `BucketContext::OnMemoryDump` dumps in-flight memory.
2. `TransactionalLevelDBDatabase::OnMemoryDump` dumps db memory size + leveldb file name.Both of the above emit to the same identifier presumably so that in-flight memory can be mapped to the origin (gleanable from the levelDB file name that appears against the db memory entry).
In the SQLite case, the existing `DatabaseMemoryDumpProvider::OnMemoryDump` dumps useful sizes under the "IndexedDB_connection" component, with the tag being the address of the specific instance.
It would be good to:
- Sum and report all those individual db sizes under the `site_storage/index_db` component (similar to (2) for LevelDB above) per origin (BucketContext instance), and
- Continue providing some attribution from opaque identifiers to the origin, similar to (1) for LevelDB above.
Abhishek ShanthkumarThanks @abhishek.s...@microsoft.com! I want to make sure I implement the intended dump shape.
For SQLite, do you want a single bucket-scoped total dump like `site_storage/indexed_db/sqlite_0x<bucket>` whose size is the sum of all open SQLite connections for that `BucketContext`, plus per-connection child dumps linked/suballocated to the existing `sqlite/IndexedDB_connection/0x...` dumps for attribution?
Also, when you say "per origin (BucketContext instance)", should this be scoped per `BucketContext` rather than adding up all buckets for the same origin?
Xiaohan ZhaoYep, that sounds right structure-wise, thanks! We can iterate on the names (it kind of makes sense not to have "sqlite" in the name under indexed_db, but keeping it "db" to match LevelDB is also misleading) subsequently.
Also, when you say "per origin (BucketContext instance)", should this be scoped per BucketContext rather than adding up all buckets for the same origin?
`BucketContext` is one per origin (with the potential exception of special cases such as third-party origin embeddings), but yes, I do specifically mean one entry per `BucketContext`.
Thanks for verifying this! The structure I'm seeing locally in chrome://tracing looks like these.
// site_storage/indexed_db/
// sqlite_0x<bucket> (total, + file_name for origin)
// connection_0x<conn> (per-connection, suballoc → sqlite/IndexedDB_connection/0x?)
I also uploaded the screenshots to the related [bug](https://issues.chromium.org/512908976#attachment76932310).
Please let me know if I should adjust anything. Thanks!
Related details
- Abhishek Shanthkumar
- Evan Stade
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar added 8 comments![]( "Open in Gerrit")
Thanks! Some feedback on the high-level approach below.
- `sqlite_0x<bucket>`: total SQLite memory across all open connections for the bucket. Includes a file_name attribute (non-background) for originnit: limit each line to 72 chars (you can just use the Format option while editing the commit message in Gerrit).
`sql::Database::ReportMemoryUsage()` is restored to power theRight, the [code comment on `DatabaseMemoryDumpProvider::ReportMemoryUsage`](https://source.chromium.org/chromium/chromium/src/+/main:sql/database_memory_dump_provider.h;l=44;bpv=1;bpt=1) refers to a call by `sql::Database` that does not exist currently. Were we able to find the commit that removed the call, just in case it has additional context that would be interesting to know?
"sqlite/IndexedDB_connection/0x?",The comment at the top of the file says "... providers can be added here only if the background mode dump has very little processor and memory overhead." Do we know that this sqlite trace has low overhead? If we're unsure, we should probably exclude considering that no other sqlite traces have been allowlisted currently.
const std::string& dump_name) {}We should make this pure virtual (`= 0`) and add a comment with empty body in the leveldb backing store explaining why it doesn't implement this.
total_size += connection->ReportMemoryUsage(
pmd, base::StringPrintf("%s/connection_0x%" PRIXPTR, dump_name.c_str(),
reinterpret_cast<uintptr_t>(connection.get())));I believe we also need to `AddOwnershipEdge`s from the `sqlite_0x` dump name to the individual `connection_0x` dumps, without which this size and the individual sizes will be counted twice in overall memory usage. Note how the LevelDB backing store does the same (in `TransactionalLevelDBDatabase::OnMemoryDump`).
dump->AddString("file_name", "", directory_.AsUTF8Unsafe());perhaps we can just log the base name of the directory. But I wonder if we can somehow log the sanitized origin itself (if available with BucketContext) instead of this directory name. Is there precedence in other dump providers to log the origin?
// Reports memory usage into provided memory dump with the given name.vnit
```suggestion
// Reports memory usage into the provided memory dump with the given name.
```
Related details
- Steve Becker
- 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. |