Bugzilla

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #74)
> (In reply to Marco Bonardo [:mak] from comment #72)
> The IPC approach definitely enables a cleaner decoupling for the Off Main Thread (OMT) storage connections, but I don't think it's worth the implementation complexity to mozStorage or to the OMT consumers (places, Quota Manager clients, rust code) given the limited benefits.

Yes, benefits are a bit limited, I agree. The implementation complexity would all be contained in the storage service and connection anyway, as far as the connections are using its methods.

> While I think the goal of about:memory continues to be noble and it fills an important niche, it's significantly less important for investigating and solving problems thanks to the amazing [Firefox Profiler](https://profiler.firefox.com/docs/#/) which has [cool memory capabilities](https://profiler.firefox.com/docs/#/./memory-allocations). 

That's a fair point.

> In order to minimize the reduction in fidelity 
> - Use `sqlite3_memory_used()` post [sqlite3_init in our AutoSQLiteLifetime helper](https://searchfox.org/mozilla-central/rev/5d287de1f7e8618dcade5cc7d480e76fae99f79b/toolkit/xre/AutoSQLiteLifetime.cpp#141) to capture the fixed memory costs of SQLite prior to us opening any connections.   Save this off to report as its own explicit value like "explicit/storage/sqlite/config" in the existing reporter.

That'd would be a static measurement though, we'd not know if that memory has been released then. A telemetry probe would be more fit.

> - Also use `sqlite3_memory_used()` to report the aggregate SQLite memory usage but not under "explicit/" so it shows up under "Other Measurements" and doesn't need to sum perfectly.

I like this idea, report whole memory as Other, single connections in explicit, and then one can subtract manually.

>   - That said, for performance reasons I could also see [turning off the global memory measurements that power this since they induce global lock contention](https://www.sqlite.org/malloc.html#_memory_status) whereas the per-connection usage does not.

That's an intersting idea and a nice finding. Maybe we could keep it only enabled in certain channels, like Nightly and early beta.

> - For main-thread connections we still use the existing reporter mechanism otherwise as I propose above.  Honestly my greatest concern would be newly added databases via Sqlite.sys.mjs, so since this should still cover them, that seems good.

I'd prefer to not differenciate too much the handling between different kind of connections.

> - We leave it to OMT connections to be reported by their owners.  This would let QuotaManager report memory usage on a per-principal basis which could ideally be lumped in with the in-flight IPC messages which would be the most interesting and time-varying data.  (Although something that the Firefox profiler can already help with.)

I'd probably move the measurement as it is today to each connection as a first step, allowing to customize the measurement could be a second step. Also because we don't have a clear ETA on when that may happen.

> #### Shutdown Spinning
> I think we've now actually moved to each component handling its own shutdown, with most adopting use of the async shutdown service at this time (ex: [SQLite.sys.mjs](https://searchfox.org/mozilla-central/source/toolkit/modules/Sqlite.sys.mjs)), or others still doing their own event loop spinning (QuotaManager uses a dedicated "profile-before-change-qm").  So this somewhat moots the original intent.

Yes, most consumers of async connections should be using blockers today.
There may be tests not doing that to fix, included cpp tests.

> And if there is code depending on this xpcom-shutdown-threads logic... it probably shouldn't and should explicitly move to using a shutdown blocker that waits for proper shutdown? 

Yes, that's part of the investigation about what will break.

> #### Verifying connections are closed
> I'd definitely be interested if you think there are hygiene benefits to this logic that I'm missing.  And if they exist, if you think requiring code to provide a proof-of-shutdown-blocker would be even better.  I like approaches like that a lot because they make the people writing the code more immediately aware of the necessity of something rather than having bugs filed against infrastructure components like mozStorage if something appears and having to work backwards from there.

My main concern is that closing the connection ensures a final merge to the database happens, leaving an empty journal, that avoids corruption if the user moves around a .sqlite file without its journal.
For connections created on the main-thread, we could probably debug assert in the connections itself that we don't receive "xpcom-shutdown-threads" (so we start observing it on open, and stop observing it on close).
For OMT connections maybe the proof of blocker is the way to go, if you have suggestions for an API, those would be welcome.

It sounds like we have some kind of plan anyway, so I'll start filing bug and hopefully find some spare time in the next weeks to start doing some of it.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #74)
> (In reply to Marco Bonardo [:mak] from comment #72)
> The IPC approach definitely enables a cleaner decoupling for the Off Main Thread (OMT) storage connections, but I don't think it's worth the implementation complexity to mozStorage or to the OMT consumers (places, Quota Manager clients, rust code) given the limited benefits.

Yes, benefits are a bit limited, I agree. The implementation complexity would all be contained in the storage service and connection anyway, as far as the connections are using its methods.

> While I think the goal of about:memory continues to be noble and it fills an important niche, it's significantly less important for investigating and solving problems thanks to the amazing [Firefox Profiler](https://profiler.firefox.com/docs/#/) which has [cool memory capabilities](https://profiler.firefox.com/docs/#/./memory-allocations). 

That's a fair point.

> In order to minimize the reduction in fidelity 
> - Use `sqlite3_memory_used()` post [sqlite3_init in our AutoSQLiteLifetime helper](https://searchfox.org/mozilla-central/rev/5d287de1f7e8618dcade5cc7d480e76fae99f79b/toolkit/xre/AutoSQLiteLifetime.cpp#141) to capture the fixed memory costs of SQLite prior to us opening any connections.   Save this off to report as its own explicit value like "explicit/storage/sqlite/config" in the existing reporter.

That'd would be a static measurement though, we'd not know if that memory has been released then. A telemetry probe would be more fit.

> - Also use `sqlite3_memory_used()` to report the aggregate SQLite memory usage but not under "explicit/" so it shows up under "Other Measurements" and doesn't need to sum perfectly.

I like this idea, report whole memory as Other, single connections in explicit, and then one can subtract manually.

>   - That said, for performance reasons I could also see [turning off the global memory measurements that power this since they induce global lock contention](https://www.sqlite.org/malloc.html#_memory_status) whereas the per-connection usage does not.

That's an intersting idea and a nice finding. Maybe we could keep it only enabled in certain channels, like Nightly and early beta.

> - For main-thread connections we still use the existing reporter mechanism otherwise as I propose above.  Honestly my greatest concern would be newly added databases via Sqlite.sys.mjs, so since this should still cover them, that seems good.

I'd prefer to not differenciate too much the handling between different kind of connections.

> - We leave it to OMT connections to be reported by their owners.  This would let QuotaManager report memory usage on a per-principal basis which could ideally be lumped in with the in-flight IPC messages which would be the most interesting and time-varying data.  (Although something that the Firefox profiler can already help with.)

I'd probably move the measurement as it is today to each connection as a first step, allowing to customize the measurement could be a second step. Also because we don't have a clear ETA on when that may happen.

> #### Shutdown Spinning
> I think we've now actually moved to each component handling its own shutdown, with most adopting use of the async shutdown service at this time (ex: [SQLite.sys.mjs](https://searchfox.org/mozilla-central/source/toolkit/modules/Sqlite.sys.mjs)), or others still doing their own event loop spinning (QuotaManager uses a dedicated "profile-before-change-qm").  So this somewhat moots the original intent.

Yes, most consumers of async connections should be using blockers today.
There may be tests not doing that, that should be fixed, included cpp tests.

> And if there is code depending on this xpcom-shutdown-threads logic... it probably shouldn't and should explicitly move to using a shutdown blocker that waits for proper shutdown? 

Yes, that's part of the investigation about what will break.

> #### Verifying connections are closed
> I'd definitely be interested if you think there are hygiene benefits to this logic that I'm missing.  And if they exist, if you think requiring code to provide a proof-of-shutdown-blocker would be even better.  I like approaches like that a lot because they make the people writing the code more immediately aware of the necessity of something rather than having bugs filed against infrastructure components like mozStorage if something appears and having to work backwards from there.

My main concern is that closing the connection ensures a final merge to the database happens, leaving an empty journal, that avoids corruption if the user moves around a .sqlite file without its journal.
For connections created on the main-thread, we could probably debug assert in the connections itself that we don't receive "xpcom-shutdown-threads" (so we start observing it on open, and stop observing it on close).
For OMT connections maybe the proof of blocker is the way to go, if you have suggestions for an API, those would be welcome.

It sounds like we have some kind of plan anyway, so I'll start filing bug and hopefully find some spare time in the next weeks to start doing some of it.