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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 2 comments![]( "Open in Gerrit")
- `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).
Done
`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?
Yes, here is the change removing it. Looks like just a simple clean up due to no usage. https://chromium-review.googlesource.com/c/chromium/src/+/7687491
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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 5 comments![]( "Open in Gerrit")
"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.
I'm not sure about this. I can see DatabaseMemoryDumpProvider::InMemoryDump calls sqlite3_db_status 3x per connection. I may be missing something regarding the overhead expectations. I'll remove this entry for now.
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.
Done
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`).
Good catch! Added AddOwnershipEdge here.
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?
No site_storage dump provider logs origin directly. The only site_storage precedent (LevelDB IndexedDB) uses just the directory basename, and IDB directory basenames are already hashes of the storage key, so they're effectively sanitized per-bucket identifiers. I'll switch to `directory_.BaseName().AsUTF8Unsafe()` to mirror LevelDB exactly.
// 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.
```
Done
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 9 comments![]( "Open in Gerrit")
Looking mostly good, thanks! Some suggestions on the dump names and origin attribution below. After addressing these comments, can you please post another screenshot of the final dumps on the issue?
"sqlite/IndexedDB_connection/0x?",Xiaohan ZhaoThe 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.
I'm not sure about this. I can see DatabaseMemoryDumpProvider::InMemoryDump calls sqlite3_db_status 3x per connection. I may be missing something regarding the overhead expectations. I'll remove this entry for now.
Cool, sgtm!
// Adds per-backing-store memory dumps under `dump_name`. Pure virtual so
// every backend explicitly opts in or out of memory reporting. Currently
// only the SQLite backend reports any memory usage at the bucket level; the
// LevelDB backend implements this as a no-op.This detail is unnecessary here.
// Adds per-backing-store memory dumps under `dump_name`. Pure virtual sonit: since this comment is on a class method of a backing store instance, it might be better to reframe it.
```suggestion
// Adds a memory dump for `this` under `dump_name`.
```
if (IsUsingSqlite()) {The LevelDB backing store no-ops ReportMemoryUsage, so this gate isn't necessary (see comment below about the dump name).
base::StringPrintf("site_storage/indexed_db/sqlite_0x%" PRIXPTR,Bikeshedding on the dump names a bit, I feel like the following structure would make the most sense:
Top-level: `site_storage/indexed_db/database_engine_0x<ptr>` <- used by both LevelDB and SQLite
Children: `site_storage/indexed_db/database_engine_0x%/sqlite_db_0x<ptr>` <- passed down to `sql::Database::ReportMemoryUsage()`
So this would entail changing the (misleading) `site_storage/indexed_db/db` prefix in `TransactionalLevelDBDatabase::OnMemoryDump()` to `site_storage/indexed_db/database_engine`. Since we are already changing `index_db` to `indexed_db`, this seems OK to do.
WDYT?
// TransactionalLevelDBDatabase::OnMemoryDump, which is registered separately
// with the MemoryDumpManager and keyed by GetIdentifierForMemoryDump(). OnlyThis detail too isn't necessary here.
// with the MemoryDumpManager and keyed by GetIdentifierForMemoryDump(). Only
// the SQLite backend needs to add per-bucket dumps under `dump_name` here.Unnecessary comment
// Provide origin attribution similar to LevelDB's `db_0x?` dump. Only
// exposed in non-background dumps, since file paths aren't allowlisted.
// The basename encodes a hash of the bucket/storage key (via
// `GetSqliteDbDirectory(bucket_locator)`), making it a sanitized per-bucket
// identifier without exposing the full path.
if (args.level_of_detail !=
base::trace_event::MemoryDumpLevelOfDetail::kBackground &&
!directory_.empty()) {
dump->AddString("file_name", "", directory_.BaseName().AsUTF8Unsafe());
}On second thought, let's punt this for now. The folder name does not give the correct picture for non-default buckets, and origin attribution is better done at the BucketContext level. Can you please file a bug and link it in a TODO in `BucketContext::OnMemoryDump` regarding adding origin attribution?
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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 9 comments![]( "Open in Gerrit")
Thanks for the comment! I've uploaded the screenshots here. https://issues.chromium.org/512908976#attachment77743024. Please let me know if that works. Thanks.
`sql::Database::ReportMemoryUsage()` is restored to power theXiaohan ZhaoRight, 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?
Yes, here is the change removing it. Looks like just a simple clean up due to no usage. https://chromium-review.googlesource.com/c/chromium/src/+/7687491
Done
// Adds per-backing-store memory dumps under `dump_name`. Pure virtual so
// every backend explicitly opts in or out of memory reporting. Currently
// only the SQLite backend reports any memory usage at the bucket level; the
// LevelDB backend implements this as a no-op.This detail is unnecessary here.
Done
// Adds per-backing-store memory dumps under `dump_name`. Pure virtual sonit: since this comment is on a class method of a backing store instance, it might be better to reframe it.
```suggestion
// Adds a memory dump for `this` under `dump_name`.
```
Done
The LevelDB backing store no-ops ReportMemoryUsage, so this gate isn't necessary (see comment below about the dump name).
Done
base::StringPrintf("site_storage/indexed_db/sqlite_0x%" PRIXPTR,Bikeshedding on the dump names a bit, I feel like the following structure would make the most sense:
Top-level: `site_storage/indexed_db/database_engine_0x<ptr>` <- used by both LevelDB and SQLite
Children: `site_storage/indexed_db/database_engine_0x%/sqlite_db_0x<ptr>` <- passed down to `sql::Database::ReportMemoryUsage()`So this would entail changing the (misleading) `site_storage/indexed_db/db` prefix in `TransactionalLevelDBDatabase::OnMemoryDump()` to `site_storage/indexed_db/database_engine`. Since we are already changing `index_db` to `indexed_db`, this seems OK to do.
WDYT?
LGTM!
// TransactionalLevelDBDatabase::OnMemoryDump, which is registered separately
// with the MemoryDumpManager and keyed by GetIdentifierForMemoryDump(). OnlyThis detail too isn't necessary here.
Done
// with the MemoryDumpManager and keyed by GetIdentifierForMemoryDump(). Only
// the SQLite backend needs to add per-bucket dumps under `dump_name` here.Xiaohan ZhaoUnnecessary comment
Done
// Provide origin attribution similar to LevelDB's `db_0x?` dump. Only
// exposed in non-background dumps, since file paths aren't allowlisted.
// The basename encodes a hash of the bucket/storage key (via
// `GetSqliteDbDirectory(bucket_locator)`), making it a sanitized per-bucket
// identifier without exposing the full path.
if (args.level_of_detail !=
base::trace_event::MemoryDumpLevelOfDetail::kBackground &&
!directory_.empty()) {
dump->AddString("file_name", "", directory_.BaseName().AsUTF8Unsafe());
}On second thought, let's punt this for now. The folder name does not give the correct picture for non-default buckets, and origin attribution is better done at the BucketContext level. Can you please file a bug and link it in a TODO in `BucketContext::OnMemoryDump` regarding adding origin attribution?
- Abhishek Shanthkumar
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
for (const auto& [name, connection] : open_connections_) {
connection->ReportMemoryUsage(
pmd, base::StringPrintf("%s/sqlite_db_0x%" PRIXPTR, dump_name.c_str(),
reinterpret_cast<uintptr_t>(connection.get())));@abhishek.s...@microsoft.com Adding ownership edges here actually hits CHECK, because `ProcessMemoryDump::AddOwnershipEdge()` only permits one outgoing ownership edge per source GUID, and multiple open SQLite connections trigger a CHECK. To avoid double-counting, I removed the aggregate scalar from the bucket-level `database_engine_0x` dump; it is now only an organizational container, while the per-connection `sqlite_db_0x` dumps carry the actual sizes. WDYT? Please let me know if that works.
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 voted and added 5 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Code-Review | +1 |
5 comments
LGTM, thanks! We should also get a memory-infra expert to take a look so we can be sure we're using the pieces correctly. I guess it will anyway be needed as part of ownership review of the allowlist file.
// Create the bucket-level dump as an organizational container. Eachnit: "bucket-level" is less appropriate IMO since this class is downstream of the whole bucket concept. Can just be:
```suggestion
// Create the dump as an organizational container.
```
// Create the bucket-level dump as an organizational container. Each
// per-connection child created below adds its own scalar and a suballocation
// ownership edge to `sqlite/IndexedDB_connection/0x?`.This too is unnecessary detail that does not belong here (I ask agents to aggressively cut down on the verbosity of comments and use existing code comments as a guide, and that seems to help to some extent).
connection->ReportMemoryUsage(This need not return the size now if we are not aggregating manually.
for (const auto& [name, connection] : open_connections_) {
connection->ReportMemoryUsage(
pmd, base::StringPrintf("%s/sqlite_db_0x%" PRIXPTR, dump_name.c_str(),
reinterpret_cast<uintptr_t>(connection.get())));@abhishek.s...@microsoft.com Adding ownership edges here actually hits CHECK, because `ProcessMemoryDump::AddOwnershipEdge()` only permits one outgoing ownership edge per source GUID, and multiple open SQLite connections trigger a CHECK. To avoid double-counting, I removed the aggregate scalar from the bucket-level `database_engine_0x` dump; it is now only an organizational container, while the per-connection `sqlite_db_0x` dumps carry the actual sizes. WDYT? Please let me know if that works.
Interesting that in the screenshots, the total for the organizational container is automatically being shown by the tracing UI itself without us having to add the size explicitly. Am I reading that right? If so, then this is definitely correct and a better approach than manually aggregating the sizes like we were doing in PS6.
>actually hits CHECK, because ProcessMemoryDump::AddOwnershipEdge() only permits one outgoing ownership edge per source GUID
But which CHECK is this? The comments above `AddOwnershipEdge()` and the code itself seems to indicate that a target node can have multiple sources (in which case the cost will be split across them). And there does not seem to be any restriction on one source owning multiple targets (which is the case with suballocations too).
Also, the [doc to adding traces](https://source.chromium.org/chromium/chromium/src/+/main:docs/memory-infra/adding_memory_infra_tracing.md) talks about using `CreateSharedGlobalAllocatorDump()` when creating a source dump. Is that something we should explore/use?
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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 3 comments![]( "Open in Gerrit")
// Create the bucket-level dump as an organizational container. Eachnit: "bucket-level" is less appropriate IMO since this class is downstream of the whole bucket concept. Can just be:
```suggestion
// Create the dump as an organizational container.
```
Fix applied.
// Create the bucket-level dump as an organizational container. Eachnit: "bucket-level" is less appropriate IMO since this class is downstream of the whole bucket concept. Can just be:
```suggestion
// Create the dump as an organizational container.
```
Fix applied.
for (const auto& [name, connection] : open_connections_) {
connection->ReportMemoryUsage(
pmd, base::StringPrintf("%s/sqlite_db_0x%" PRIXPTR, dump_name.c_str(),
reinterpret_cast<uintptr_t>(connection.get())));Abhishek Shanthkumar@abhishek.s...@microsoft.com Adding ownership edges here actually hits CHECK, because `ProcessMemoryDump::AddOwnershipEdge()` only permits one outgoing ownership edge per source GUID, and multiple open SQLite connections trigger a CHECK. To avoid double-counting, I removed the aggregate scalar from the bucket-level `database_engine_0x` dump; it is now only an organizational container, while the per-connection `sqlite_db_0x` dumps carry the actual sizes. WDYT? Please let me know if that works.
Interesting that in the screenshots, the total for the organizational container is automatically being shown by the tracing UI itself without us having to add the size explicitly. Am I reading that right? If so, then this is definitely correct and a better approach than manually aggregating the sizes like we were doing in PS6.
>actually hits CHECK, because ProcessMemoryDump::AddOwnershipEdge() only permits one outgoing ownership edge per source GUID
But which CHECK is this? The comments above `AddOwnershipEdge()` and the code itself seems to indicate that a target node can have multiple sources (in which case the cost will be split across them). And there does not seem to be any restriction on one source owning multiple targets (which is the case with suballocations too).
Also, the [doc to adding traces](https://source.chromium.org/chromium/chromium/src/+/main:docs/memory-infra/adding_memory_infra_tracing.md) talks about using `CreateSharedGlobalAllocatorDump()` when creating a source dump. Is that something we should explore/use?
Yes, you're reading the screenshots correctly. The UI rolls up the children under the `database_engine_0x...` container., so we don't need to add an explicit aggregate size.
The CHECK I hit is in `ProcessMemroyDump::AddOwnershipEdge()`: https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/process_memory_dump.cc?q=process_memory_dump&ss=chromium%2Fchromium%2Fsrc#:~:text=if%20(it%20!%3D%20allocator_dumps_edges_,%7D. Although multiple sources can own the same target, edges are stored by source GUID, and adding the same source with a different target hits `DCHECK_EQ(target.ToUint64(), it->second.target.ToUint64())`. The SQLite parent-to-many-children shape would use one `database_engine_0x...` source with multiple `sqlite_db_0x...` targets, which violates that invariant.
I don’t think `CreateSharedGlobalAllocatorDump()` is needed here because the SQLite DB dumps are not modeling a separately shared global allocation; `sql::Database::ReportMemoryUsage()` already creates the concrete per-DB dump and suballocates it to the SQLite memory tracker. The parent `database_engine_0x...` is only an organizational container.
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. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 2 comments![]( "Open in Gerrit")
// Create the bucket-level dump as an organizational container. Each
// per-connection child created below adds its own scalar and a suballocation
// ownership edge to `sqlite/IndexedDB_connection/0x?`.This too is unnecessary detail that does not belong here (I ask agents to aggressively cut down on the verbosity of comments and use existing code comments as a guide, and that seems to help to some extent).
Done
This need not return the size now if we are not aggregating manually.
Done
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
for (const auto& [name, connection] : open_connections_) {
connection->ReportMemoryUsage(
pmd, base::StringPrintf("%s/sqlite_db_0x%" PRIXPTR, dump_name.c_str(),
reinterpret_cast<uintptr_t>(connection.get())));Abhishek Shanthkumar@abhishek.s...@microsoft.com Adding ownership edges here actually hits CHECK, because `ProcessMemoryDump::AddOwnershipEdge()` only permits one outgoing ownership edge per source GUID, and multiple open SQLite connections trigger a CHECK. To avoid double-counting, I removed the aggregate scalar from the bucket-level `database_engine_0x` dump; it is now only an organizational container, while the per-connection `sqlite_db_0x` dumps carry the actual sizes. WDYT? Please let me know if that works.
Xiaohan ZhaoInteresting that in the screenshots, the total for the organizational container is automatically being shown by the tracing UI itself without us having to add the size explicitly. Am I reading that right? If so, then this is definitely correct and a better approach than manually aggregating the sizes like we were doing in PS6.
>actually hits CHECK, because ProcessMemoryDump::AddOwnershipEdge() only permits one outgoing ownership edge per source GUID
But which CHECK is this? The comments above `AddOwnershipEdge()` and the code itself seems to indicate that a target node can have multiple sources (in which case the cost will be split across them). And there does not seem to be any restriction on one source owning multiple targets (which is the case with suballocations too).
Also, the [doc to adding traces](https://source.chromium.org/chromium/chromium/src/+/main:docs/memory-infra/adding_memory_infra_tracing.md) talks about using `CreateSharedGlobalAllocatorDump()` when creating a source dump. Is that something we should explore/use?
Yes, you're reading the screenshots correctly. The UI rolls up the children under the `database_engine_0x...` container., so we don't need to add an explicit aggregate size.
The CHECK I hit is in `ProcessMemroyDump::AddOwnershipEdge()`: https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/process_memory_dump.cc?q=process_memory_dump&ss=chromium%2Fchromium%2Fsrc#:~:text=if%20(it%20!%3D%20allocator_dumps_edges_,%7D. Although multiple sources can own the same target, edges are stored by source GUID, and adding the same source with a different target hits `DCHECK_EQ(target.ToUint64(), it->second.target.ToUint64())`. The SQLite parent-to-many-children shape would use one `database_engine_0x...` source with multiple `sqlite_db_0x...` targets, which violates that invariant.
I don’t think `CreateSharedGlobalAllocatorDump()` is needed here because the SQLite DB dumps are not modeling a separately shared global allocation; `sql::Database::ReportMemoryUsage()` already creates the concrete per-DB dump and suballocates it to the SQLite memory tracker. The parent `database_engine_0x...` is only an organizational container.
- Abhishek Shanthkumar
- Etienne Pierre-Doray
- Greg Thompson
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Abhishek Shanthkumar
| Code-Review | +1 |
3 comments
// Create the bucket-level dump as an organizational container. Eachnit: "bucket-level" is less appropriate IMO since this class is downstream of the whole bucket concept. Can just be:
```suggestion
Xiaohan Zhao// Create the dump as an organizational container.
```
Abhishek ShanthkumarFix applied.
Looks like this got missed.
for (const auto& [name, connection] : open_connections_) {
connection->ReportMemoryUsage(
pmd, base::StringPrintf("%s/sqlite_db_0x%" PRIXPTR, dump_name.c_str(),
reinterpret_cast<uintptr_t>(connection.get())));Abhishek Shanthkumar@abhishek.s...@microsoft.com Adding ownership edges here actually hits CHECK, because `ProcessMemoryDump::AddOwnershipEdge()` only permits one outgoing ownership edge per source GUID, and multiple open SQLite connections trigger a CHECK. To avoid double-counting, I removed the aggregate scalar from the bucket-level `database_engine_0x` dump; it is now only an organizational container, while the per-connection `sqlite_db_0x` dumps carry the actual sizes. WDYT? Please let me know if that works.
Xiaohan ZhaoInteresting that in the screenshots, the total for the organizational container is automatically being shown by the tracing UI itself without us having to add the size explicitly. Am I reading that right? If so, then this is definitely correct and a better approach than manually aggregating the sizes like we were doing in PS6.
>actually hits CHECK, because ProcessMemoryDump::AddOwnershipEdge() only permits one outgoing ownership edge per source GUID
But which CHECK is this? The comments above `AddOwnershipEdge()` and the code itself seems to indicate that a target node can have multiple sources (in which case the cost will be split across them). And there does not seem to be any restriction on one source owning multiple targets (which is the case with suballocations too).
Also, the [doc to adding traces](https://source.chromium.org/chromium/chromium/src/+/main:docs/memory-infra/adding_memory_infra_tracing.md) talks about using `CreateSharedGlobalAllocatorDump()` when creating a source dump. Is that something we should explore/use?
Xiaohan ZhaoYes, you're reading the screenshots correctly. The UI rolls up the children under the `database_engine_0x...` container., so we don't need to add an explicit aggregate size.
The CHECK I hit is in `ProcessMemroyDump::AddOwnershipEdge()`: https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/process_memory_dump.cc?q=process_memory_dump&ss=chromium%2Fchromium%2Fsrc#:~:text=if%20(it%20!%3D%20allocator_dumps_edges_,%7D. Although multiple sources can own the same target, edges are stored by source GUID, and adding the same source with a different target hits `DCHECK_EQ(target.ToUint64(), it->second.target.ToUint64())`. The SQLite parent-to-many-children shape would use one `database_engine_0x...` source with multiple `sqlite_db_0x...` targets, which violates that invariant.
I don’t think `CreateSharedGlobalAllocatorDump()` is needed here because the SQLite DB dumps are not modeling a separately shared global allocation; `sql::Database::ReportMemoryUsage()` already creates the concrete per-DB dump and suballocates it to the SQLite memory tracker. The parent `database_engine_0x...` is only an organizational container.
Done
one ... source with multiple ... targets ... violates that invariant.
Hmm, this is certainly weird. I would expect that one source can own multiple targets. Perhaps the recommendation is to use suballocations in that case.
Anyway, thanks for checking!
if (!db_ || !db_->ReportMemoryUsage(pmd, dump_name)) {
return;
}The null-check isn't necessary because `DatabaseConnection` does not outlive its `db_` member.
```suggestion
db_->ReportMemoryUsage(pmd, dump_name);
```
Related details
- Etienne Pierre-Doray
- Greg Thompson
- 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. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
1 comment
//sql lgtm. please wait for etiennep to review //base. thanks.
Related details
- Etienne Pierre-Doray
- 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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
1 comment
- Steve Becker
- Xiaohan Zhao
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
if (!db_ || !db_->ReportMemoryUsage(pmd, dump_name)) {
return;
}The null-check isn't necessary because `DatabaseConnection` does not outlive its `db_` member.
```suggestion
db_->ReportMemoryUsage(pmd, dump_name);
```
Thanks for the review! Since I'm not a Chromium committer yet, uploading a new patch set would reset the existing approvals, I'm going to land the current CL first since this seems non-blocking, and I'll address this in a small follow-up CL and add you as a reviewer very soon. Thanks again!
Related details
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Zhao (Gerrit)
Xiaohan Zhao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Xiaohan Zhao (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
IndexedDB: Add memory dump reporting for SQLite backing store
Add `site_storage/indexed_db/` memory dumps for SQLite-backed IndexedDB
buckets:
- `sqlite_0x<bucket>`: total SQLite memory across all open connections
for the bucket. Includes a file_name attribute (non-background) for
origin attribution.
- `sqlite_0x<bucket>/connection_0x<conn>`: per-connection
child dump, suballocated to the existing
`sqlite/IndexedDB_connection/0x?` dump for cross-tree attribution.
Both paths are added to the background allowlist.
`sql::Database::ReportMemoryUsage()` is restored to power the
per-connection reporting via `DatabaseMemoryDumpProvider`. A
`ReportMemoryUsage()` virtual method is added to the `BackingStore`
interface.
- M base/trace_event/memory_infra_background_allowlist.cc
- M components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.cc
- M content/browser/indexed_db/instance/backing_store.h
- M content/browser/indexed_db/instance/bucket_context.cc
- M content/browser/indexed_db/instance/leveldb/backing_store.cc
- M content/browser/indexed_db/instance/leveldb/backing_store.h
- M content/browser/indexed_db/instance/sqlite/backing_store_impl.cc
- M content/browser/indexed_db/instance/sqlite/backing_store_impl.h
- M content/browser/indexed_db/instance/sqlite/database_connection.cc
- M content/browser/indexed_db/instance/sqlite/database_connection.h
- M sql/database.cc
- M sql/database.h
Code-Review: +1 by Greg Thompson, +1 by Etienne Pierre-Doray, +1 by Abhishek Shanthkumar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |