Issue metadata
Sign in to add a comment
|
52 KB regression in resource_sizes (MonochromePublic.apk) at 572055:572055 |
||||||||||||||||||||
Issue descriptionCaused by “Use a huffman trie for top domain storage in url formatter” Commit: 9e93f9821f9fa0611d345710a55e694d8ec7234e Link to size graph: https://chromeperf.appspot.com/report?sid=bb23072657e2d7ca892a1c3fa4643b1ee29b3a0a44d0732adda87168e89c0380&num_points=10&rev=572055 Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase
,
Jul 5
Assigning to meacer@google.com because this is the only CL in range: Use a huffman trie for top domain storage in url formatter UrlFormatter uses ICU's spoof checker to determine lookalike domains that contain unicode confusables. It does this by extracting a skeleton string from the given domain representing its visual appearance. For example, google.com and googlé[.]com have the same skeleton string (google.corn). In addition to this, we want to display a "Did you mean to go to..." UI for navigations involving IDN if the domain name matches a top 10K domain. In order to do that, we need to store the domains associated with ICU skeletons. UrlFormatter currently uses a DAFSA to store the list of the skeletons of the top 10K domains. It doesn't and cannot store the actual domain in this list. To support this, this CL changes the underlying storage from DAFSA to the Huffman Trie used by net's preload list code. It - Generates the huffman trie from top domain list during compile time. - Decodes the huffman trie during runtime in IDNSpoofChecker::SimilarToTopDomains. The design doc for the preload list migration is here: https://docs.google.com/document/d/11rqIozUDaK6DvNeu436SL3Coj65J5vhD9HVftOi-RrA/edit As mentioned in the doc, micro benchmarks indicate that the binary size and speed is minimally impacted by this change (51KB additional size, 4 microseconds of additional time for each lookup). Bug: 843361 Change-Id: If98b8161bf836fec6ba74e68587bd2159f4eb3d5 Reviewed-on: https://chromium-review.googlesource.com/1106539 Commit-Queue: Mustafa Emre Acer <meacer@chromium.org> Reviewed-by: Nick Harper <nharper@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#572055}
,
Jul 5
Partial symbol diff:
Section Sizes (Total=52.0kb (53285 bytes)):
.bss: 0 bytes (0 bytes) (not included in totals)
.data: 32 bytes (32 bytes) (0.1%)
.data.rel.ro: 32 bytes (32 bytes) (0.1%)
.dex: 0 bytes (0 bytes) (0.0%)
.other: 2.53kb (2590 bytes) (4.9%)
.pak.nontranslated: 0 bytes (0 bytes) (0.0%)
.pak.translations: 0 bytes (0 bytes) (0.0%)
.rel.dyn: 7 bytes (7 bytes) (0.0%)
.rodata: 49.2kb (50432 bytes) (94.6%)
.text: 192 bytes (192 bytes) (0.4%)
12 symbols added (+), 448 changed (~), 3 removed (-), 998947 unchanged (not shown)
Of changed symbols, 17 grew, 446 shrank
Number of unique symbols 629064 -> 629072 (+8)
0 paths added, 0 removed, 324 changed
Showing 463 symbols (14 -> 22 unique) with total pss: 53277 bytes
Histogram of symbols based on PSS:
(-65536,-32768]: 1 (-16,-8]: 2 (0,1): 1 [8,16): 2 [64,128): 2 [65536,131072): 1
(-128,-64]: 2 (-8,-4]: 1 [1,2): 2 [16,32): 4 [128,256): 1
(-64,-32]: 1 (-1,0): 439 [2,4): 1 [32,64): 2 [2048,4096): 1
.text=191 bytes .rodata=49.2kb .data.rel.ro=32 bytes .data=32 bytes .bss=0 bytes .dex=0 bytes .dex.method=0 bytes .pak.translations=0 bytes .pak.nontranslated=0 bytes .other=2.53kb total=52.0kb
Number of unique paths: 321
Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, x=.dex, m=.dex.method, p=.pak.translations, P=.pak.nontranslated, o=.other
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
+ 0) 109890 (206.3%) r@0x4b1b89 109890 (0->109890) components/url_formatter/idn_spoof_checker.cc
url_formatter::kTopDomainsTrie
- 1) 50401 (94.6%) r@0x0 -59489 (59489->0) components/url_formatter/idn_spoof_checker.cc
url_formatter::kDafsa
+ 2) 50557 (94.9%) t@0x1754618 156 (0->156) components/url_formatter/idn_spoof_checker.cc
url_formatter::TopDomainPreloadDecoder::ReadEntry
- 3) 50477 (94.7%) t@0x0 -80 (80->0) net/extras/preload_data/decoder.cc
HuffmanDecoder::Decode
+ 4) 50557 (94.9%) t@0x167478e 80 (0->80) net/extras/preload_data/decoder.cc
net::extras::PreloadDecoder::HuffmanDecoder::Decode const
+ 5) 50637 (95.0%) r@0x4b1b39 80 (0->80) components/url_formatter/idn_spoof_checker.cc
url_formatter::kTopDomainsHuffmanTree
~ 6) 50569 (94.9%) t@0x1674814 -68 (482->414) net/extras/preload_data/decoder.cc
net::extras::PreloadDecoder::Decode
~ 7) 50621 (95.0%) t@0x17544f8 52 (236->288) components/url_formatter/idn_spoof_checker.cc
url_formatter::LookupMatchInTopDomains
+ 8) 50661 (95.1%) t@0x1674766 40 (0->40) net/extras/preload_data/decoder.cc
net::extras::PreloadDecoder::BitReader::Seek
+ 9) 50685 (95.1%) d@0x29adf98 24 (0->20) components/url_formatter/idn_spoof_checker.cc
url_formatter::g_trie_params
+ 10) 50705 (95.2%) R@0x2a70180 20 (0->20) components/url_formatter/idn_spoof_checker.cc
url_formatter::TopDomainPreloadDecoder [vtable]
~ 11) 50721 (95.2%) t@0x16747e0 16 (36->52) net/extras/preload_data/decoder.cc
net::extras::PreloadDecoder::PreloadDecoder
~ 12) 50733 (95.2%) o@0x0 12 (2011786->2011798) base/trace_event/cfi_backtrace_android.cc
assets/unwind_cfi_32
- 13) 50725 (95.2%) d@0x0 -8 (8->0) components/url_formatter/idn_spoof_checker.cc
.L_MergedGlobals
~ 14) 50728 (95.2%) o@0x0 +3 (73175->73178) $APK/META-INF/MANIFEST.MF
META-INF/MANIFEST.MF
~ 15) 50729 (95.2%) o@0x0 +1 (1008->1009) $APK/META-INF/CHROMIUM.RSA
META-INF/CHROMIUM.RSA
~ 16) 50730 (95.2%) o@0x0 +1 (74441->74442) $APK/META-INF/CHROMIUM.SF
META-INF/CHROMIUM.SF
+ 17) 50730 (95.2%) t@0x635108 ~0 (0->~0) components/url_formatter/idn_spoof_checker.cc
url_formatter::TopDomainPreloadDecoder::~TopDomainPreloadDecoder (num_aliases=440)
~ 18) 53315 (100.1%) o@0x0 2585 (0->0) {no path}
Overhead: ELF file
+ 19) 53266 (100.0%) r@0x0 -49 (0->0) {no path}
** aggregate padding of diff'ed symbols
+ 20) 53282 (100.0%) d@0x0 16 (0->0) {no path}
** aggregate padding of diff'ed symbols
+ 21) 53294 (100.0%) R@0x0 12 (0->0) {no path}
** aggregate padding of diff'ed symbols
~ 22) 53282 (100.0%) o@0x0 -12 (0->0) {no path}
Overhead: APK file
+ 23) 53278 (100.0%) t@0x0 -4 (0->0) {no path}
** aggregate padding of diff'ed symbols
~ 24) 53278 (100.0%) t@Group ~0 (~0->~0) chrome/browser/signin/account_consistency_mode_manager.cc
AccountConsistencyModeManager::~AccountConsistencyModeManager (count=2)
~ 25) 53278 (100.0%) t@0x635108 ~0 (~0->~0) chrome/browser/android/ntp/android_content_suggestions_notifier.cc
AndroidContentSuggestionsNotifier::~AndroidContentSuggestionsNotifier (num_aliases=439->440)
~ 26) 53278 (100.0%) t@Group ~0 (~0->~0) chrome/browser/metrics/android_metrics_provider.cc
,
Jul 5
It's not clear to me whether or not this increase was expected. Please have a look and either: Close as “Won't Fix” with a short justification, or Land a revert / fix-up.
,
Jul 5
Looks like you've already looked into this quite a bit.
,
Jul 9
Yes, this is expected as we are now storing more data. Design doc that discusses the benchmarks: https://docs.google.com/document/d/11rqIozUDaK6DvNeu436SL3Coj65J5vhD9HVftOi-RrA/edit |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 5