Lots of hashing of the same extension ID |
||||||
Issue descriptionAs part of my work on https://crbug.com/621530 I put a VLOG() into base::SHA1HashString() and found the following: On boot I see: localhost ~ # grep -c SHA-1 /var/log/chrome/chrome 1389 If I login and open a page I see something like: localhost ~ # grep -c SHA-1 /home/user/eef763a43f010f7b084145c0152551763f3136d6/log/chrome 4070 If I now look at the system log (after 5 minutes of capturing the others): localhost ~ # grep -c SHA-1 /var/log/chrome/chrome 28052 After 40 minutes of uptime (single page open, otherwise "idle") localhost ~ # grep -c SHA-1 /var/log/chrome/chrome 45456 localhost ~ # grep SHA-1 /var/log/chrome/chrome | cut -f 3 -d " " | sort | uniq -c | sort -n | tail -n 5 1892 pmfjbimdmchhbnneeidfognadeopoehp 2060 fgoepimhcoialccpbmpnnblemnepkkao 2098 pafkbggdmjlpgkdkcbjmhmfcdpncadgh 2323 hhaomjibdihmijegdhdafkllkbggdgoj 3929 nckgahadagoaajjgafhacjanaoiihapd In the user log there are 6 calls to hash nckgahadagoaajjgafhacjanaoiihapd (the ID for Google Hangouts extension) every 20s. A page load gives around 26 hash attempts (the majority again being nckgahadagoaajjgafhacjanaoiihapd) so the page load is not something to worry about specifically. The backtrace I obtained from gdb shows these extension IDs coming in through extensions::IsIdInArray()/IsIdInList() I'm going to try and refactor to reduce the numbers of calls we make to base::SHA1HashString().
,
Jul 4 2016
I tried pulling the hashing out to a higher level in https://codereview.chromium.org/2116293003 I'm not sure that the readability impact is worth the performance benefit. Marking as Available if somebody else wants to go further.
,
Jul 4 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 17 2017
,
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 should be fixed with #6. Thanks for the report! |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by rsleevi@chromium.org
, Jun 21 2016Labels: Performance