Extension feature whitelisting calculation is wasteful |
|||
Issue descriptionWe check feature availability (and as a part of that, the whitelists and blacklists) very frequently in extensions code. Currently, checking each list consists of: 1. Checking the list for the raw extension id 2. Hashing the extension id and hex-encoding the hash 3. Checking the list for the hashed/hexed id This is very wasteful, given how frequently we do it. We can simplify this: For 1. We shouldn't check for the raw extension id For 2. We can cache the hashed/encoded id (perhaps even on the Extension object) Depending on the costs we see, we could also pursue a FeatureCache-like solution (https://chromium.googlesource.com/chromium/src/+/c0ec1bba917bc9790356c5c93b531cf788313d4f/extensions/renderer/feature_cache.h), but that may or may not be worthwhile. Assigning to myself, but happy to share the work!
,
Aug 17 2017
,
Aug 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6 commit 8a7267ff95c4955a5b25b25a9fa636acda2ac8f6 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Mon Aug 21 17:58:46 2017 [Extensions Features] Add a HashedExtensionId construct Add a simple HashedExtensionId class as a wrapper around a hex-encoded SHA1 hash of an extension id, and store this in the Manifest class (with the regular extension id). This allows us to compute the hash of an extension id once, rather than recomputing it each time we calculate feature availability. Previously, we would calculate this hash each time we'd examine a feature for availability - which happens during extension initialization and loading, bindings set up, function execution and more. (This was even worse because we would check the blacklist even if the list was empty, resulting in calculating the hash for that case no matter what). Fun Stats (local sampling, so your mileage may vary): Before, opening Chromium with default extensions resulted in ~350 calls to crx_file::id_util::HashedIdInHex() in both the browser process *and* each renderer process (so a minimum of ~700 calls to just open chrome, with additional tabs triggering an additional ~350 calls). Now, opening Chromium with default extensions results in <10 calls to crx_file::id_util::HashedIdInHex() in each process. With an average run speed (on a debug build [slow] of a Z840 [fast] - so maybe representative-ish of real situations?) of 0.034ms in the browser process and 0.303ms in the renderer process, this corresponds to a 11.9ms speed up in browser startup and 106ms speed up in renderer startup. Note that each extension adds to this linearly, so if the user had an extension with content scripts running on each page, this would save ~700 renderer calls, resulting in a 212ms speed up. Yay! Bug: 622035 Bug: 755441 Change-Id: Idd5655982ad03ab55882d7b44a177a41fe1cb02c Reviewed-on: https://chromium-review.googlesource.com/619663 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Cr-Commit-Position: refs/heads/master@{#495984} [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/chrome/browser/extensions/api/activity_log_private/activity_log_private_api.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/BUILD.gn [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/extension.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/extension.h [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/complex_feature.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/complex_feature.h [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/complex_feature_unittest.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/feature.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/feature.h [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/simple_feature.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/simple_feature.h [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/features/simple_feature_unittest.cc [add] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/hashed_extension_id.cc [add] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/hashed_extension_id.h [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/manifest.cc [modify] https://crrev.com/8a7267ff95c4955a5b25b25a9fa636acda2ac8f6/extensions/common/manifest.h
,
Aug 25 2017
This is way better than it was, and we've seen a correlated speed-up in extension initialization time. Closing this out. |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Aug 16 2017