New issue
Advanced search Search tips

Issue 813792 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

55kb regression in resource_sizes (MonochromePublic.apk) at 537119:537119

Project Member Reported by agrieve@chromium.org, Feb 20 2018

Issue description

Caused by “Added AboutUI page for connection-help.”

Commit: 766fbc88bed0c117536f00aab036aef7851a3c03

Link to size graph: https://chromeperf.appspot.com/report?sid=a097e74b1aa288511afb4cb616efe0f95ba4d347ad61d5e835072f23450938ba&num_points=10&rev=537119 

Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase

Based on the graph: 8kb of non-translated .pak, and 58k of localized .pak (not sure why this adds to more than 55kb...)

It looks like this increase might be avoidable.
Please have a look and either:

Close as “Won't Fix” with a short justification, or
Land a revert / fix-up.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Feb 20 2018

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

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=3eb1187315c3695fb37fd2ae6fbfd9d50ebba89b110ae11cc5380655174d911a


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

Android Builder
Note: Since the size increase is all in .pak, I'd guess that we could save a lot by enabling gzip on the newly added resources.

The CSS & JS is pretty straight-forward to add compress="gzip" and then ensure that the loading code is set to decompress when loading (many examples of this).

The HTML might be a bit trickier, since I'm guessing the loading path is a bit different, but please have a shot at it. Adding compress="gzip" should be enough to enable compression within the .pak, but loading it might be more involved.

Comment 3 Deleted

Quick clarification about the CSS and JS, currently the offline help content does not use any JS, and for CSS the CL did not add any new CSS, it's reusing the interstitial CSS (and using the default WebUI CSS from AppendWebUiCssTextDefaults) so changing the retrieval would mean also changing it for interstitials, which can be done if needed, but I'd like to make sure that won't cause other performance regressions from the decompression time. 

For the HTML, I'll take a look to see if compress="gzip" works with that.
Here's some of the output from: tools/binary_size/diagnose_bloat.py --cloud 766fbc88bed0c117536f00aab036aef7851a3c03

Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, p=.pak.translations, P=.pak.nontranslated, o=.other
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
+ 0)      52400 (79.9%) p@0x3e07     52400 (0->52400)   chrome/browser/ui/webui/about_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_HTML
+ 1)      59843 (91.3%) P@0x3f3c     7443 (0->7443)     chrome/browser/ui/webui/about_ui.cc
               ../../components/resources/security_interstitials_resources.grdp: IDR_SECURITY_INTERSTITIAL_COMMON_CSS

So, looks like even though it existed already, this is first time IDR_SECURITY_INTERSTITIAL_COMMON_CSS is being used on Android. Certainly true that the HTML is the majority of the increase though.

Most websites use gzip transfer encoding, so I don't think it should be an issue for our internals pages to use it. We've enabled compression on many resources already and no metrics have regressed that I know about.
I see now, the CSS was exposed (and loaded) as a string for the first time by the code in my CL, since it was previously just loaded directly from the html.

Adding the compress flag on this one might be tricky since the html is loaded directly as a string from the GRD file where it's stored (and as far as I can see from other grd files, adding compress="gzip" to a grd string is not an option), while the CSS is loaded from that grdp file. So setting source()->UseGzip() wouldn't work here since the CSS part would be gzipped but the HTML would not.

Is there something I'm missing here, or would the only option be to move the HTML to its own file and load it as a resource?
Ah, I think you're right that you need to use an <include> to have compress="gzip" work. Moving it to its own file seems simple, but would that break translations? Might need to add support for compress="gzip" to string nodes.
I'm trying to figure out how translations for .html files get done (I can't find any documentation on that, but at least chrome://terms is done that way, and it is translated), once I figure that out I'll move this to a file and add the compression flag.
Status: Started (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e8d78880849513e050f3dc5e77a360701e4dad1

commit 8e8d78880849513e050f3dc5e77a360701e4dad1
Author: Carlos IL <carlosil@chromium.org>
Date: Wed Mar 21 18:23:36 2018

Moved chrome://connection-help logic to a separate WebUI.

Moved connection-help out of AboutUI and into its own WebUI, also
separated strings in the grd file, and implemented the content as an
HTML template that is filled with those strings.
Compression was also enabled for resources.

Bug:  813792 
Change-Id: Ibc3e86206a7662490570cecff4a304030ba767f2
Reviewed-on: https://chromium-review.googlesource.com/957877
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Edward Jung <edwardjung@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544775}
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/chrome/browser/ui/webui/about_ui.cc
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/chrome/common/webui_url_constants.cc
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/chrome/common/webui_url_constants.h
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/resources/security_interstitials_resources.grdp
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/BUILD.gn
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/connection_help_ui.cc
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/connection_help_ui.h
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/resources/connection_help.css
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/resources/connection_help.html
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/resources/connection_help.js
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/urls.cc
[add] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials/content/urls.h
[modify] https://crrev.com/8e8d78880849513e050f3dc5e77a360701e4dad1/components/security_interstitials_strings.grdp

Status: Fixed (was: Started)
That revision should address this, the strings were moved to grd files, and compress="gzip" was added for the HTML, CSS, and JS templates.
Thanks for your efforts. Looks like it didn't actually help much. Here's the graph for the commit from #10:

https://chromeperf.appspot.com/report?sid=6e2075e36f226f182a202af67eb08fce78c2b1890c5af1418e839c91d50d24ad&rev=544775

This initial increase was 55kb. The reduction was only 4kb.

From a run of: tools/binary_size/diagnose_bloat.py 8e8d78880849513e050f3dc5e77a360701e4dad1 --cloud:

.text=-352 bytes .rodata=-271 bytes .data.rel.ro=0 bytes    .data=0 bytes    .bss=0 bytes    .pak.translations=-204 bytes .pak.nontranslated=-4.14kb    .other=-7 bytes   total=-4.96kb
Number of unique paths: 8

Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, p=.pak.translations, P=.pak.nontranslated, o=.other
Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path
------------------------------------------------------------
- 0)     -50660 (997.6%) p@0x0        -50660 (50660->0)  chrome/browser/ui/webui/about_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_HTML
+ 1)     -31015 (610.8%) p@0x3e52     19645 (0->19645)   components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_CONNECTION_NOT_PRIVATE_DETAILS
+ 2)     -19661 (387.2%) p@0x3e55     11354 (0->11354)   components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_MITM_SOFTWARE_DETAILS
~ 3)     -24958 (491.5%) P@0x42ac     -5297 (7443->2146) components/security_interstitials/content/connection_help_ui.cc
               ../../components/resources/security_interstitials_resources.grdp: IDR_SECURITY_INTERSTITIAL_COMMON_CSS
+ 4)     -20602 (405.7%) p@0x3e4c     4356 (0->4356)     components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_GENERAL_HELP
+ 5)     -16595 (326.8%) p@0x3e4e     4007 (0->4007)     components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_CONNECTION_NOT_PRIVATE_TITLE
+ 6)     -13503 (265.9%) p@0x3e53     3092 (0->3092)     components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_CONNECT_TO_NETWORK_DETAILS
+ 7)     -10658 (209.9%) p@0x3e54     2845 (0->2845)     components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_INCORRECT_CLOCK_DETAILS
+ 8)      -8960 (176.4%) p@0x3e50     1698 (0->1698)     components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_INCORRECT_CLOCK_TITLE
+ 9)      -7349 (144.7%) p@0x3e51     1611 (0->1611)     components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_MITM_SOFTWARE_TITLE
+ 10)     -6711 (132.2%) p@0x3e4d     638 (0->638)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_SPECIFIC_ERROR_HEADING
+ 11)     -6108 (120.3%) P@0x42b1     603 (0->603)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/resources/security_interstitials_resources.grdp: IDR_SECURITY_INTERSTITIAL_CONNECTION_HELP_HTML
+ 12)     -5716 (112.6%) p@0x3e4b     392 (0->392)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_HEADING
+ 13)     -5339 (105.1%) p@0x3e4f     377 (0->377)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_CONNECT_TO_NETWORK_TITLE
+ 14)     -4996 (98.4%) P@0x42b2     343 (0->343)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/resources/security_interstitials_resources.grdp: IDR_SECURITY_INTERSTITIAL_CONNECTION_HELP_CSS
+ 15)     -4663 (91.8%) P@0x42b3     333 (0->333)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/resources/security_interstitials_resources.grdp: IDR_SECURITY_INTERSTITIAL_CONNECTION_HELP_JS
~ 16)     -4959 (97.7%) t@0x2667fa8  -296 (1132->836)   chrome/browser/ui/webui/about_ui.cc
               AboutUIHTMLSource::StartDataRequest
+ 17)     -5188 (102.2%) r@0x0        -229 (0->0)        {{no path}}
               ** aggregate padding of diff'ed symbols
~ 18)     -5409 (106.5%) P@0x42ad     -221 (604->383)    components/security_interstitials/content/connection_help_ui.cc
               ../../components/resources/security_interstitials_resources.grdp: IDR_SECURITY_INTERSTITIAL_CORE_CSS
+ 19)     -5192 (102.2%) p@0x3e49     217 (0->217)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_SHOW_LESS
+ 20)     -4975 (98.0%) p@0x3e48     217 (0->217)       components/security_interstitials/content/connection_help_ui.cc
               ../../components/security_interstitials_strings.grdp: IDS_CONNECTION_HELP_SHOW_MORE
- 21)     -5062 (99.7%) r@0x0        -87 (87->0)        chrome/browser/ui/webui/about_ui.cc
               string literal
~ 22)     -5118 (100.8%) t@0x2668540  -56 (2332->2276)   chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
               GetWebUIFactoryFunction
~ 23)     -5070 (99.8%) r@0x3d76e4   48 (10756->10804)  chrome/browser/metrics/variations/ui_string_overrider_factory.cc
               chrome_variations::kResourceHashes
~ 24)     -5022 (98.9%) r@0x3da118   48 (10756->10804)  chrome/browser/metrics/variations/ui_string_overrider_factory.cc
               chrome_variations::kResourceIndices
- 25)     -5058 (99.6%) r@0x0        -36 (36->0)        chrome/browser/ui/webui/about_ui.cc
               string literal
- 26)     -5074 (99.9%) r@0x0        -16 (16->0)        chrome/common/webui_url_constants.cc
               chrome::kChromeUIConnectionHelpHost


Looks like the savings are from things that are now gzip, but overall I think there's just a lot of text on the page, and not much that can be done.

One small possibility is putting IDS_CONNECTION_HELP_MITM_SOFTWARE_DETAILS behind (#if OS_WINDOWS), since it's says "(windows only)" in the message. But if a use-case for this page includes looking up help for another computer, then we should just leave it in.



That's unfortunate. The windows section is no longer included on other platforms as of crrev.com/c/988838, so there might be another (small) decrease on size from that change as you suggested. Thanks for the suggestion.

Sign in to add a comment