Issue metadata
Sign in to add a comment
|
55kb regression in resource_sizes (MonochromePublic.apk) at 537119:537119 |
||||||||||||||||||||
Issue descriptionCaused 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.
,
Feb 20 2018
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.
,
Feb 20 2018
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.
,
Feb 21 2018
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.
,
Feb 21 2018
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?
,
Feb 22 2018
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.
,
Feb 26 2018
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.
,
Mar 16 2018
,
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
,
Mar 26 2018
That revision should address this, the strings were moved to grd files, and compress="gzip" was added for the HTML, CSS, and JS templates.
,
Mar 26 2018
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.
,
Apr 5 2018
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 |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Feb 20 2018