Issue metadata
Sign in to add a comment
|
0.7% regression in sizes at 405351:405355 |
||||||||||||||||||||
Issue descriptionSuspecting this change: commit ec678d0e5493256ccf2b2b6a752de289b58b9d28 author jinsukkim <jinsukkim@chromium.org> Wed Jul 13 23:55:22 2016 committer Commit bot <commit-bot@chromium.org> Wed Jul 13 23:58:18 2016 Reland "Replace ICU with CED for auto encoding detection"
,
Jul 19 2016
A new third-party library (third_party/ced) hooked up in the CL caused this - not an unexpected increase in the size. CED didn't completely replace ICU since ICU is still being used in other places. The library comes with benefit of much faster auto-detection of text encoding, which I believe justifies the small regression in the size.
,
Jul 19 2016
+grt (sizes owner on Windows) and laforge: see #2
,
Jul 19 2016
That's a fairly large size regression, is there anything that can be removed from ICU to offset the size increase?
,
Jul 19 2016
Now that I see it, the size delta is quite big. For Android, the delta was only 90KB https://bugs.chromium.org/p/chromium/issues/detail?id=597488#c18 Let me build Windows binary before and after to make sure if the CL is really the culprit.
,
Jul 20 2016
While waiting for my modest Window laptop to build the binaries, I checked out Linux binaries - the size increases about 254KB. On Windows it's around 388KB. Though bigger on Windows platform, I think it may be right to say that the regression can be attributed to the CL. I was hoping to replace ICU encoding detection lib completely by CED but could only go as far as replacing use cases in Blink. Chrome still needs ICU encoding detection. So I'm afraid there is no part removable from ICU to even out the regression. Let me check out further why on Window we see bigger regression. There may be some build flags to tweak on MSVC to alleviate the impact.
,
Jul 22 2016
I was wrong on #6 about not being able to replace ICU encoding detection entirely. It is possible - now WIP https://codereview.chromium.org/2168003003 I suspect this will shave ~100KB off the final binary.
,
Aug 12 2016
,
Aug 12 2016
The CL landed but it surprisingly increased the size further. See Issue 636886 Which means the ICU files (third_party/icu/source/i18n/ucsdet.{cpp|h}, third_party/icu/source/i18n/csdetect.{cpp|h}) are not used anymore but still around as a part of the dll. Maybe they should explicitly be removed from the build.
,
Aug 18 2016
(The CL mentioned in #c9 is https://crrev.com/2168003003) Tested with local builds but deleting the icu files from source list doesn't help decrease the size at all - both give a binary of exactly same size. Looks like those unused object files are already removed at linking phase. I don't have other idea at this moment. The better performance of the new encoding detection library comes at a cost of the binary size increase which may be inevitable.
,
Aug 19 2016
I'm expecting https://crrev.com/2254273003 which becomes feasible thanks to the introduction of third_party/ced to mitigate the size regression.
,
Aug 31 2016
Committed at @415497, decreased the size by 25KB - a baby step towards mitigating the regression.
,
Sep 9 2016
jinsukkim@, could you update this bug with what the next steps are?
,
Sep 19 2016
I think a potential improvement may come from optimizing the size of the new library in conjunction with Issue 647582 . I don't have a concrete plan at this moment as it needs closer look into it.
,
Oct 11 2016
Perf sheriff ping
,
Nov 24 2016
,
Jan 4 2017
While doing some unrelated investigations I found a VC++ compiler bug that was triggering code bloat and private-page bloat in CED. A few extra 'const' modifiers were causing a huge initializer function to be created, which also meant that the ~53 KB of data ended up being per-process private instead of shared. The fix is awaiting approval and will reduce the size of chrome.dll and chrome_child.dll by a combined 908,288 bytes: https://github.com/google/compact_enc_det/pull/4 That suggests that the 90 KB increase on Android is what we should be expecting, which makes the increase on Linux also suspicious. I've documented the techniques I've been using to investigate binary size regressions and waste: https://www.chromium.org/developers/windows-binary-sizes
,
Jan 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c62b45ea72fa4a9f57bf25ead85db4c397e43e5d commit c62b45ea72fa4a9f57bf25ead85db4c397e43e5d Author: brucedawson <brucedawson@chromium.org> Date: Wed Jan 04 18:38:54 2017 Roll src\third_party\ced\src/ e57cdc44b..368a9cc09 (1 commit). https://chromium.googlesource.com/external/github.com/google/compact_enc_det.git/+log/e57cdc44bd54..368a9cc09ad8 $ git log e57cdc44b..368a9cc09 --date=short --no-merges --format='%ad %ae %s' 2016-12-29 brucedawson Save 908,288 bytes by deleting 'const' three times TBR=jinsukkim@chromium.org BUG= 677351 , 629332 Review-Url: https://codereview.chromium.org/2617463003 Cr-Commit-Position: refs/heads/master@{#441416} [modify] https://crrev.com/c62b45ea72fa4a9f57bf25ead85db4c397e43e5d/DEPS
,
Jan 4 2017
,
Feb 1 2017
Issue 683393 has been merged into this issue. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by m...@chromium.org
, Jul 18 2016