Optimization: use StartupCache to skip database access in ExtensionScriptingStore when there are no persisted scripts
Categories
(WebExtensions :: General, enhancement, P1)
Tracking
(firefox105 fixed, firefox106 fixed)
People
(Reporter: robwu, Assigned: rpl)
References
(Blocks 1 open bug)
Details
(Whiteboard: [addons-jira])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
Currently, the implementation delays extension initialization until the ExtensionScriptingStore has read from the rkv database. This may result in a non-negligible delay. The scripting
namespace is not only used for persistently registered content scripts, so that behavior is suboptimal.
We should use the StartupCache to avoid expensive delays at https://searchfox.org/mozilla-central/rev/db4b1d66c4b409bdbce43f3f3498401f5303d961/toolkit/components/extensions/Extension.jsm#3106
This StartupCache data could be maintained at https://searchfox.org/mozilla-central/rev/db4b1d66c4b409bdbce43f3f3498401f5303d961/toolkit/components/extensions/ExtensionScriptingStore.jsm#243,285-287,292
Because the StartupCache format is wiped when the application version changes, we don't need to worry about data migrations, and can afford to prioritize ease/speed of implementation over complexity that may be more involved.
For example:
- Just saving a flag that tells whether there are any persisted scripts can already offer performance benefits to extensions that don't use persistent content scripts.
- Caching the whole set of registered content scripts can offer performance benefits of all extensions including extensions with persisted scripts, but requires a more careful design to avoid the cached and persisted data getting out-of-sync.
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
Comment 3•2 years ago
|
||
bugherder |
Assignee | ||
Comment 4•2 years ago
|
||
Comment on attachment 9290967 [details]
Bug 1785898 - Cache into StartupCache a boolean flag to determine if there is any persisted content script. r?mixedpuppy!,robwu
Beta/Release Uplift Approval Request
- User impact if declined: Potential impact on the user perceived performance on startup due to waiting on the underlying rkv store to load all data related to content scripts registered as
persistAcrossSession: true
(if a user has an extension installed which is requesting the "scripting" permission and registering webRequest blocking listeners for web content requests intercepted during the browser startup).
As a site note about Android builds:
- Android builds would be expected to be potentially affected as a Desktop build would be, given that the scripting API and the area where the changes in this patch are applied are both at
toolkit/
level (and so shared by both Desktop and Android builds). - The xpcshell test covering the fix is expected to also be running on Android builds (and so to cover the expected behaviors on both Desktop and Android builds).
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): - The change is quite small and pretty much restricted to the ExtensionScriptingStore.jsm, which is only used on extensions that have access to the scripting API.
-
Both the scripting API and the persistAcrossSession option have been introduced recently enough (and so they are not too commonly used yet by the current versions of the top most installed extensions).
-
The fix have been baking for a few days in Nightly, manually verify the fix is a bit tricky (especially given that there may not be many existing extensions already using these features) but a new test (included in the same patch) is explicitly asserting the expected behaviors.
- String changes made/needed:
- Is Android affected?: Yes
Comment 5•2 years ago
|
||
Am I understanding correctly that the only users incurring the startup performance penalty are those using addons which use the scripting API? Or is everyone hitting it regardless of which extensions they have installed?
Assignee | ||
Comment 6•2 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
Am I understanding correctly that the only users incurring the startup performance penalty are those using addons which use the scripting API? Or is everyone hitting it regardless of which extensions they have installed?
Yes, that is correct, the patch from Bug 1751436 as landed in Firefox 105 only calls await lazy.ExtensionScriptingStore.initExtension(this);
if this.hasPermission("scripting")
:
Comment 7•2 years ago
|
||
Comment on attachment 9290967 [details]
Bug 1785898 - Cache into StartupCache a boolean flag to determine if there is any persisted content script. r?mixedpuppy!,robwu
Avoid a startup performance penalty introduced in 105 for those who are using addons which use the scripting API. Approved for 105.0b5.
Comment 8•2 years ago
|
||
bugherder uplift |
Description
•