New issue
Advanced search Search tips

Issue 629332 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression

Blocked on:
issue 647582



Sign in to add a comment

0.7% regression in sizes at 405351:405355

Project Member Reported by m...@chromium.org, Jul 18 2016

Issue description

Suspecting 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"


 

Comment 1 by m...@chromium.org, Jul 18 2016

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=629332

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg8ufArwoM


Bot(s) for this bug's original alert(s):

win
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.
Cc: lafo...@chromium.org grt@chromium.org
+grt (sizes owner on Windows) and laforge: see #2
That's a fairly large size regression, is there anything that can be removed from ICU to offset the size increase?
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.
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.
Status: Started (was: Assigned)
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.
Cc: jinsuk...@chromium.org
 Issue 636886  has been merged into this issue.
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.
(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.
I'm expecting https://crrev.com/2254273003 which becomes feasible thanks to the introduction of third_party/ced to mitigate the size regression. 
Committed at @415497, decreased the size by 25KB - a baby step towards mitigating the regression.
jinsukkim@, could you update this bug with what the next steps are?
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. 

Perf sheriff ping
Blockedon: 647582
What's the ETA for a concrete plan?
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
Project Member

Comment 18 by bugdroid1@chromium.org, 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

Owner: brucedaw...@chromium.org
Status: Fixed (was: Started)
 Issue 683393  has been merged into this issue.

Sign in to add a comment