Add memory dumping support for IndexedDB SQLite connections. [chromium/src : main]
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
Hi folks, PTAL when you have a chance, thanks!
Related details
- 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. |
Evan Stade (Gerrit)
Evan Stade added 5 comments![]( "Open in Gerrit")
self to cc, Abhishek ptal.
Bug: 40253999can you file a more specific bug for this please?
"site_storage/index_db/db_0x?",nit: these should be indexed_db
"site_storage/index_db/memenv_0x?",memenv and db both seem unused? When did that become the case? Can we fix this?
static_cast<sqlite::BackingStoreImpl*>(backing_store())all interaction with the backing store (except construction) should go through the backing store interface, meaning this cast should not be necessary.
Related details
- Abhishek Shanthkumar
- 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. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar added 4 comments![]( "Open in Gerrit")
Thanks for taking this up! I had just started looking into it after seeing your other CL for DomStorage. Glad you are on it.
Add memory dumping support for IndexedDB SQLite connections.Can we briefly describe the name and structure of the memory dumps being added?
"site_storage/index_db/memenv_0x?",memenv and db both seem unused? When did that become the case? Can we fix this?
They 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).
static_cast<sqlite::BackingStoreImpl*>(backing_store())all interaction with the backing store (except construction) should go through the backing store interface, meaning this cast should not be necessary.
Adding 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.
Related details
- Steve Becker
- Xiaohan Zhao
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
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.
Adding 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.
Thanks @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?
Related details
- Abhishek Shanthkumar
- Steve Becker
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. |
Abhishek Shanthkumar (Gerrit)
Abhishek Shanthkumar added 1 comment![]( "Open in Gerrit")
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.
Thanks @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?
Yep, 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`.
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. |