chrome_child.dll grew 5MB recently (and nobody noticed?) |
||||||
Issue descriptionSee https://chromeperf.appspot.com/report?sid=7778cd057883709478caf1f8ed5af083422aefb955c6e80c573bb29c51afc8ed&start_rev=465181&end_rev=466768 Looks like this change: https://codereview.chromium.org/2762593002 I assume we should revert that. But the second question is how this was only noticed by myself and not some automatic perf system?
,
Apr 24 2017
,
Apr 24 2017
re:reverting https://crrev.com/d238dae172d63175b562f99fc988e47e5213a244 I think that we have landed a couple cls that depend on this already, so reverting could be tricky. It should be a matter of removing unnecessary libraries at headless child lib https://cs.chromium.org/chromium/src/headless/BUILD.gn?rcl=84e360a7f1dddd9ce125ad6f1de090b7d207c569&l=591 I think the most straightforward solution would be to land a separate cl to bring this down in the next two days. wdyt?
,
Apr 24 2017
I don't think chrome_child is linking headless_shell_lib?
,
Apr 24 2017
Oops sorry, it's linking headless_shell_child_lib: https://cs.chromium.org/chromium/src/headless/BUILD.gn?rcl=84e360a7f1dddd9ce125ad6f1de090b7d207c569&l=562
,
Apr 24 2017
Oh, sorry headless_shell_child_lib. I don't obviously see what's causing the increase.
,
Apr 24 2017
The change only landed a bit over a weekend ago. If it's already difficult to revert, let's do that while it's still possible.
,
Apr 24 2017
FWIW I agree with #7 if there is a way to revert, even if it involves multiple reverts, we should do so.
,
Apr 24 2017
I'm trying a dry run: https://codereview.chromium.org/2837093003/
,
Apr 24 2017
Yup, I agree on revert too, hopefully just reverting a few CLs in reverse order?
fwiw, SymbolSort didn't enlighten me:
c:\stuff>dir 466124\chrome-win32\chrome_child.dll
Volume in drive C has no label.
Volume Serial Number is 88D1-EBAA
Directory of c:\stuff\466124\chrome-win32
2017-04-20 02:26 PM 66,627,072 chrome_child.dll
1 File(s) 66,627,072 bytes
0 Dir(s) 238,838,808,576 bytes free
c:\stuff>dir 466266\chrome-win32\chrome_child.dll
Volume in drive C has no label.
Volume Serial Number is 88D1-EBAA
Directory of c:\stuff\466266\chrome-win32
2017-04-20 10:58 PM 75,011,072 chrome_child.dll
1 File(s) 75,011,072 bytes
0 Dir(s) 238,838,808,576 bytes free
c:\stuff>C:\src\SymbolSort\bin\Release\SymbolSort.exe -diff 466124\chrome-win32\chrome_child.dll -in 466266\chrome-win32\chrome_child.dll -out diff.txt
Loading symbols from 466266\chrome-win32\chrome_child.dll
Reading section info...
Reading source file info...
Reading global function symbols... 100% complete
Reading thunk symbols... 100% complete
Reading private data symbols... 100% complete
Reading global data symbols... 100% complete
Subtracting overlapping symbols... 100
Loading symbols from 466124\chrome-win32\chrome_child.dll
Reading section info...
Reading source file info...
Reading global function symbols... 100% complete
Reading thunk symbols... 100% complete
Reading private data symbols... 100% complete
Reading global data symbols... 100% complete
Subtracting overlapping symbols... 100
Processing raw symbols...
Building folder stats...
Computing section stats...
Merging duplicate symbols...
Merging template symbols...
Merging overloaded symbols...
Building tag cloud...
Elapsed Time: 00:00:12.7682445
,
Apr 24 2017
Since you have the binaries handy, what does this say? python tools\win\pe_summarize.py c:\stuff\466124\chrome-win32\chrome_child.dll c:\stuff\466266\chrome-win32\chrome_child.dll It could be good to know what section is showing the growth.
,
Apr 24 2017
whole lotta code
c:\stuff>python c:\src\cr\src\tools\win\pe_summarize.py c:\stuff\466124\chrome-win32\chrome_child.dll c:\stuff\466266\chrome-win32\chrome_child.dll
Size of c:\stuff\466124\chrome-win32\chrome_child.dll is 66.627072 MB
name: mem size , disk size
.text: 49.876187 MB
.rdata: 13.051306 MB
.data: 1.843076 MB, 0.536576 MB
.pdata: 2.526792 MB
.tls: 0.000045 MB
.rodata: 0.005648 MB
.gfids: 0.003192 MB
_RDATA: 0.032816 MB
.rsrc: 0.052400 MB
.reloc: 0.538000 MB
Size of c:\stuff\466266\chrome-win32\chrome_child.dll is 75.011072 MB
name: mem size , disk size
.text: 57.123275 MB
.rdata: 13.752514 MB
.data: 1.876004 MB, 0.543744 MB
.pdata: 2.914560 MB
.tls: 0.000045 MB
.rodata: 0.005648 MB
.gfids: 0.003192 MB
_RDATA: 0.032816 MB
.rsrc: 0.052400 MB
.reloc: 0.578616 MB
Memory size change from c:\stuff\466124\chrome-win32\chrome_child.dll to c:\stuff\466266\chrome-win32\chrome_child.dll
.text: 7247088 bytes change
.rdata: 701208 bytes change
.data: 32928 bytes change
.pdata: 387768 bytes change
.reloc: 40616 bytes change
Total change: 8409608 bytes
,
Apr 24 2017
This makes no sense. I feel like I must have the wrong pdbs somehow. [ I got them from https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Win_x64/466124/ and 466266/. ] Maybe I'll try official signed builds instead.
,
Apr 25 2017
Oh, continuous are probably symbol_level=1. Doh. Gimme a minute or 5.
,
Apr 25 2017
OK, I pulled 60.0.3077.0 and 60.0.3078.0, got chrome_child.dll out of them, and got the pdbs for those via symchk /v.
c:\stuff\off>dir /s
...
Directory of c:\stuff\off\3077
2017-04-24 04:56 PM <DIR> .
2017-04-24 04:56 PM <DIR> ..
2017-04-21 12:21 PM 61,403,992 chrome_child.dll
2017-04-21 03:10 AM 1,497,083,904 chrome_child.dll.pdb
2 File(s) 1,558,487,896 bytes
Directory of c:\stuff\off\3078
2017-04-24 04:56 PM <DIR> .
2017-04-24 04:56 PM <DIR> ..
2017-04-22 04:31 AM 68,366,168 chrome_child.dll
2017-04-22 04:36 AM 1,519,169,536 chrome_child.dll.pdb
...
c:\stuff\off>python c:\src\cr\src\tools\win\pe_summarize.py 3077\chrome_child.dll 3078\chrome_child.dll
Size of 3077\chrome_child.dll is 61.403992 MB
name: mem size , disk size
.text: 45.163597 MB
.rdata: 12.642828 MB
.data: 2.412004 MB, 0.475648 MB
.pdata: 2.473812 MB
.rodata: 0.009528 MB
.gfids: 0.003192 MB
.tls: 0.000045 MB
_RDATA: 0.041776 MB
.rsrc: 0.052344 MB
.reloc: 0.530020 MB
Size of 3078\chrome_child.dll is 68.366168 MB
name: mem size , disk size
.text: 51.120413 MB
.rdata: 13.261940 MB
.data: 2.441380 MB, 0.479232 MB
.pdata: 2.816676 MB
.rodata: 0.009528 MB
.gfids: 0.003192 MB
.tls: 0.000045 MB
_RDATA: 0.041776 MB
.rsrc: 0.052344 MB
.reloc: 0.570288 MB
Memory size change from 3077\chrome_child.dll to 3078\chrome_child.dll
.text: 5956816 bytes change
.rdata: 619112 bytes change
.data: 29376 bytes change
.pdata: 342864 bytes change
.reloc: 40268 bytes change
Total change: 6988436 bytes
c:\stuff\off>C:\src\SymbolSort\bin\Release\SymbolSort.exe -diff 3077\chrome_child.dll -in 3078\chrome_child.dll -out diff.txt
Loading symbols from 3078\chrome_child.dll
Reading section info...
Reading source file info...
Reading global function symbols... 100% complete
Reading thunk symbols... 100% complete
Reading private data symbols... 100% complete
Reading global data symbols... 100% complete
Subtracting overlapping symbols... 100
Loading symbols from 3077\chrome_child.dll
Reading section info...
Reading source file info...
Reading global function symbols... 100% complete
Reading thunk symbols... 100% complete
Reading private data symbols... 100% complete
Reading global data symbols... 100% complete
Subtracting overlapping symbols... 100
Processing raw symbols...
Building folder stats...
Computing section stats...
Merging duplicate symbols...
Merging template symbols...
Merging overloaded symbols...
Building tag cloud...
Elapsed Time: 00:13:48.9638129
This seems like a more useful diff. From a quick look it looks like content/browser is getting more-used now. It was already linked in via
> gn path //out/Release //chrome:chrome_child //content/browser
//chrome:chrome_child --[private]-->
//chrome:child_dependencies --[public]-->
//chrome/browser/devtools:devtools --[private]-->
//content/public/browser:browser --[public]-->
//content/public/browser:browser_sources --[private]-->
//content/browser:browser
previous to this CL though, so I might be wrong about that (or this might have just caused so things to be non-discardable). Needs more digging.
,
Apr 25 2017
Oops, wrong diff.
,
Apr 25 2017
Most of the report is too diffuse to be interesting. It seems to be best to start at this section:
Increases in Total Size
Total Size Total Count Name
29524 549 trace_event_internal::AddTraceEventWithThreadIdAndTimestamp
12164 0 sqlite3VdbeExec
10408 1 protected: virtual class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl cc::FragmentShader::GetShaderSource(void)const __ptr64
10233 1 content::`anonymous namespace'::AccessibilityEnumMap::AccessibilityEnumMap
9642 1 public: static bool __cdecl indexed_db::mojom::DatabaseStubDispatch::Accept(class indexed_db::mojom::Database * __ptr64,class mojo::Message * __ptr64)
8495 0 sqlite3Pragma
8028 26 trace_event_internal::AddTraceEventWithThreadIdAndTimestamp<std::basic_string<char,std::char_traits<char>,std::allocator<char> > >
Two of the largest newly added functions are sqlite3 functions, and searching on sqlite3 shows a bunch more. But, not enough to account for more than a tiny fraction of the increase so that's only useful if it leads to a useful thread of the ball of yarn of new code being pulled in.
,
Apr 25 2017
Thanks for the insights! I'm trying to do a manual rebase of the patch, hopefully I can submit. Can anyone LGTM the patch (https://codereview.chromium.org/2762593002/) I need a committer to approve the rollback
,
Apr 25 2017
lgtm'd (i assume you mean https://codereview.chromium.org/2837093003/)
,
Apr 25 2017
I managed to land the revert and looks like size is back to normal. I agree with #15 that it may be pulling more DEPS than it should, hopefully is an easy fix!
,
Apr 25 2017
Sometimes a CL will pull in more than expected simply because there is a long and accidental chain of other functions or data in the .obj files pulled in. That is, you call Foo(), and that's all you want, but Bar() is in the same file and it gets pulled in and it references 5 MB of other stuff. Ideally the linker would discard the excess but that doesn't always happen perfectly.
A gn-specific variant on this is caused by source sets - you link with a new source set which means you are linking with all of those files, in a stronger sense than referencing a traditional library. You just want Foo() from foo.cc but you end up pulling in Bar() from bar.cc, and all of its global variables and tangled references.
In either case the trick, if the solution is not apparent, is to find some symbol which gets pulled in but shouldn't (perhaps the sqlite3 functions) and then find out why. I've created some tools that use the VC++ verbose linker output on Windows to systematically figure out why a particular function is pulled in so that you can find and fix these issues quickly and reliably. I've documented the techniques here:
https://www.chromium.org/developers/windows-binary-sizes
If the instructions don't entirely make sense or don't work then drop me a line.
,
Apr 26 2017
Thanks for the pointers Bruce, those are really useful. Note that chrome_dll also experience an increase of 650KB (this was also noted recently in https://bugs.chromium.org/p/chromium/issues/detail?id=714888) https://chromeperf.appspot.com/report?sid=7abd71420d1c74281b5c0fb6d97750629c34de951459b18435292f2555fa5e5b&start_rev=465788&end_rev=467144 This is more in line with what would be expected of adding headless library to Chrome. So yeah I'm guessing we are pulling unnecessary dependencies. It's interesting that you mention source_set, since that's what I used to have a render (child) specific library for headless [1]. I'll see if that could be the issue. [1] source_set(headless_renderer) in https://codereview.chromium.org/2762593002/diff/560001/headless/BUILD.gn
,
Apr 26 2017
Is it understood why the perf bots didn't catch this? If not, should we have a separate bug for that?
,
Apr 26 2017
So I diffed gn desc out/Default chrome:main_child --all All the headless dependencies should be there. I manage to remove the others, hopefully that brings the increase to the expected ~650 KB @@ -314,6 +314,7 @@ //base/win:base_win_features //base/win:eventlog_messages //breakpad:breakpad_handler + //breakpad:client //build/config/freetype:freetype //build/config/nacl:nacl_base(//build/toolchain/nacl:irt_x64) //build/config/nacl:nacl_base @@ -545,6 +546,7 @@ //components/crash/content/app:app //components/crash/content/app:app_non_mac_win //components/crash/content/app:lib + //components/crash/content/browser:browser //components/crash/core/common:common //components/crash/core/common:crash_keys //components/crx_file:crx_file @@ -690,6 +692,7 @@ //components/policy/proto:proto_internal_gen //components/pref_registry:pref_registry //components/prefs:prefs + //components/printing/browser:browser //components/printing/common:common //components/printing/renderer:renderer //components/proxy_config:proxy_config @@ -724,6 +727,7 @@ //components/search_engines:prepopulated_engines //components/search_engines:prepopulated_engines_action //components/search_engines:search_engines + //components/security_state/content:content //components/security_state/core:core //components/signin/core/account_id:account_id //components/signin/core/browser:browser @@ -1095,6 +1099,20 @@ //gpu/ipc/service:service //gpu/skia_bindings:skia_bindings //gpu/vulkan:features + //headless:gen_devtools_client_api + //headless:headless + //headless:headless_renderer + //headless:headless_shell_child_lib + //headless:pak + //headless:resources + //headless:resources_grit + //headless:tab_socket + //headless:tab_socket__generator + //headless:tab_socket__type_mappings + //headless:tab_socket_shared__generator + //headless:tab_socket_shared_cpp_sources + //headless:version_header + //headless:version_header_action //ipc:ipc(//build/toolchain/nacl:irt_x64) //ipc:ipc //ipc:mojom(//build/toolchain/nacl:irt_x64) @@ -2420,4 +2438,5 @@
,
Apr 26 2017
,
Apr 28 2017
I did a diff on an official build without the extra dependencies.
Looks like largest difference comes from gen/Blink/Bindings. I'm not sure why
Looks like content/browser is still there
These are the top differences
c:\tools\SymbolSort\bin\Debug\SymbolSort.exe -in .\out\ReleaseHeadless\chrome_child.dll -diff .\out\Release\chrome_child.dll -out C:\tmp\ss.of.diff
.txt
Raw Symbols Differences
Total Count : 80060
Total Size : 7979008
Unattributed : 1056823
File Contributions
--------------------------------------
Increases in Size
Size Count Source Path
6906481 52954 c:
6821105 35338 c:\src\out\releaseheadless
6763266 35214 c:\src\out\releaseheadless\gen
6034885 43872 c:\src
4118642 25083 c:\src\out\releaseheadless\gen\blink
3223203 18353 c:\src\out\releaseheadless\gen\blink\bindings
2700121 15393 c:\src\content
2583461 12860 c:\src\content\browser
1734715 11328 c:\src\out\releaseheadless\gen\blink\bindings\core\v8
1488488 7025 c:\src\out\releaseheadless\gen\blink\bindings\modules\v8
871596 9082 c:\users\dvallet\apps\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616
864393 8986 c:\users\dvallet\apps\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\vc
857671 8938 c:\users\dvallet\apps\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\vc\include
838184 4473 c:\src\out\releaseheadless\gen\blink\core
644752 3238 c:\src\out
546127 12119 c:\src\base
522126 458 c:\src\out\releaseheadless\gen\chrome\common
521279 2914 c:\src\out\releaseheadless\gen\blink\core\inspector\protocol
467826 185 c:\src\out\releaseheadless\gen\chrome\common\extensions
465412 2151 c:\src\out\releaseheadless\gen\content
457972 152 c:\src\out\releaseheadless\gen\chrome\common\extensions\api
434848 2301 c:\src\content\browser\renderer_host
433433 3086 c:\users\dvallet\apps\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\vc\include\xtree
,
Apr 28 2017
That's really odd. I would have expected all the v8 bindings to already be there in chrome_child.dll. (+cc Kentaro in case he has ideas of where to look or who to ask.)
,
Apr 28 2017
Does this mean that v8 bindings was previously not included in chrome_child.dll? Or does it mean that v8 bindings in chrome_child.dll increased recently?
,
Apr 28 2017
David, correct me if I'm wrong but I think the symbols were already there but they just grew in size (?)
,
Apr 28 2017
Look around line 11649 in the report in comment #26:
Increases in Total Size
Total Size Total Count Name
13810 1 content::`anonymous namespace'::AccessibilityEnumMap::AccessibilityEnumMap
12093 1 public: static bool __cdecl indexed_db::mojom::DatabaseStubDispatch::Accept(class indexed_db::mojom::Database * __ptr64,class mojo::Message * __ptr64)
11085 1 protected: virtual class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl cc::FragmentShader::GetShaderSource(void)const __ptr64
9940 1 public: virtual bool __cdecl content::RenderFrameHostImpl::OnMessageReceived(class IPC::Message const & __ptr64) __ptr64
7952 1 kWebuiResources
7883 1 public: class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl cc::VertexShader::GetShaderString(void)const __ptr64
7712 1 public: static bool __cdecl leveldb::mojom::LevelDBDatabaseStubDispatch::AcceptWithResponder(class leveldb::mojom::LevelDBDatabase * __ptr64,class mojo::Message * __ptr64,class std::unique_ptr<class mojo::MessageReceiverWithStatus,struct std::default_delete<class mojo::MessageReceiverWithStatus> >)
7376 1 public: static bool __cdecl blink::mojom::WebBluetoothServiceStubDispatch::AcceptWithResponder(class blink::mojom::WebBluetoothService * __ptr64,class mojo::Message * __ptr64,class std::unique_ptr<class mojo::MessageReceiverWithStatus,struct std::default_delete<class mojo::MessageReceiverWithStatus> >)
5150 1 content::`anonymous namespace'::GpuInfoAsDictionaryValue
4944 1 private: virtual void __cdecl content::AccessibilityTreeFormatterWin::AddProperties(class content::BrowserAccessibility const & __ptr64,class base::DictionaryValue * __ptr64) __ptr64
4919 1 public: virtual bool __cdecl content::WebContentsImpl::OnMessageReceived(class content::RenderFrameHostImpl * __ptr64,class IPC::Message const & __ptr64) __ptr64
4833 1 public: virtual bool __cdecl content::RenderWidgetHostImpl::OnMessageReceived(class IPC::Message const & __ptr64) __ptr64
4816 1 private: class gfx::Rect __cdecl cc::SurfaceAggregator::PrewalkTree(class cc::SurfaceId const & __ptr64,bool,int,struct cc::SurfaceAggregator::PrewalkResult * __ptr64) __ptr64
4782 1 public: virtual bool __cdecl content::FileAPIMessageFilter::OnMessageReceived(class IPC::Message const & __ptr64) __ptr64
4750 1 YUY2ToARGBRow_C
4735 1 public: static bool __cdecl blink::mojom::PresentationServiceStubDispatch::Accept(class blink::mojom::PresentationService * __ptr64,class mojo::Message * __ptr64)
4565 1 public: static bool __cdecl ui::mojom::GpuHostStubDispatch::Accept(class ui::mojom::GpuHost * __ptr64,class mojo::Message * __ptr64)
4339 1 public: virtual void __cdecl cc::FragmentShader::Init(class gpu::gles2::GLES2Interface * __ptr64,unsigned int,int * __ptr64) __ptr64
4047 1 private: void __cdecl content::RenderProcessHostImpl::CreateMessageFilters(void) __ptr64
4020 1 public: static bool __cdecl blink::mojom::BackgroundFetchServiceStubDispatch::AcceptWithResponder(class blink::mojom::BackgroundFetchService * __ptr64,class mojo::Message * __ptr64,class std::unique_ptr<class mojo::MessageReceiverWithStatus,struct std::default_delete<class mojo::MessageReceiverWithStatus> >)
3995 1 UYVYToARGBRow_C
3878 1 NV12ToARGBRow_C
Searching for these symbols can confirm whether they grew or are brand new. content::`anonymous namespace'::AccessibilityEnumMap::AccessibilityEnumMap, for instance, appears to be brand new.
Ditto with public: static bool __cdecl indexed_db::mojom::DatabaseStubDispatch::Accept(class indexed_db::mojom::Database * __ptr64,class mojo::Message * __ptr64).
Ditto with YUY2ToARGBRow_C.
Looking at the increase by individual symbols instead of by directories seems to work better. And, it gives an actionable question: find one of the symbols that should not have been pulled in and then use tools\win\linker_verbose_tracking.py (documented at https://www.chromium.org/developers/windows-binary-sizes) to figure out why. Repeat until all mysteries are solved.
,
May 1 2017
Thanks Bruce for the pointers! I'm not sure though if I'm using the tool properly. I generated the link info (around 1.4GB) and did the following: - AccessibilityEnumMap::AccessibilityEnumMap, defined in https://cs.chromium.org/chromium/src/content/browser/accessibility/accessibility_tree_formatter_utils_win.cc?l=47 I executed the following: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.txt accessibility_tree_formatter_utils_win.obj ==> And got no results. I did notice that the objet was mention in the linker output - mojom::DatabaseStubDispatch::Accept. I executed: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.txt indexed_db.mojom.obj ==> No results Am I doing this correctly?
,
May 1 2017
It looks like the trailing ".obj" is supposed to be emitted. I meant to add a check for that but I guess I never did. Try this: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.txt accessibility_tree_formatter_utils_win If that doesn't work or if the results are inscrutable I can certainly take a look.
,
May 2 2017
It does omit the .obj. The full output is: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.txt accessibility_tree_formatter_utils_win Database loaded - 0 xrefs found No references to accessibility_tree_formatter_utils_win.obj found. Perhaps I'm missing a flag for the verbose linker. I've attached all the lines that match 'accessibility_tree_formatter_utils_win.obj' in my output.
,
May 2 2017
I *think* that what that means is that that file is specified on the command line, probably as part of a source set. The linker is pulling it in because you explicitly told it to link that file. That is the problem with source sets - the fully link all of the files contained within, and then we have to hope that the linker can discard what is not referenced later in the process. The .obj files in .lib files, in contrast, are pulled in on demand (which causes a different set of issues). Ping me if you want to discuss or I can try to reproduce your results, but it looks like the problem is that a source_set is being pulled in and the linker is unable to discard all of its contents, even though they are not logically needed. I can tweak the tool to make that clearer - you're really only the second user :-)
,
May 2 2017
0 xrefs found seems wrong - I think that means that the script didn't find anything in the verbose output. Please share it with me and I'll take a look.
,
May 2 2017
I've put it on https://x20.corp.google.com/users/dv/dvallet/chrome.child.link.txt.zip Yeah I'll look a bit more into it. But maybe I need more pointers on how to hunt for the source set
,
May 4 2017
I looked around a bit and here's what I did/found. First, I turned on verbose linking for chrome_child.dll:
diff --git a/chrome/BUILD.gn b/chrome/BUILD.gn
index a959bfb..c80a8ea 100644
--- a/chrome/BUILD.gn
+++ b/chrome/BUILD.gn
@@ -464,6 +464,7 @@ if (is_win) {
"/DELAYLOAD:dxva2.dll",
"/DELAYLOAD:esent.dll",
"/DELAYLOAD:wininet.dll",
+ "/VERBOSE",
]
if (symbol_level == 2) {
I ran your command and note that it said 12120 xrefs found. So something is wrong with your verbose linker output (since it shows zero). I don't know what it would be.
> python .\tools\win\linker_verbose_tracking.py chrome_child_00_initial.txt accessibility_tree_formatter_utils_win
Database loaded - 12120 xrefs found
No references to accessibility_tree_formatter_utils_win.obj found.
The "no references" message means that the script didn't find a symbol that caused this object file to be pulled in. So either it wasn't linked at all (we know that's not true) or else it was linked directly - i.e.; in a source_set. A quick search of *.gn* for accessibility_tree_formatter_utils_win.cc finds that it is in source_set("browser"). Okay. So it is pulled in because we explicitly requested it.
I changed "browser" to a static library and tried again:
>python tools\win\linker_verbose_tracking.py chrome_child_01_browser_to_static_library.txt accessibility_tree_formatter_utils_win
Database loaded - 12120 xrefs found
accessibility_tree_formatter_utils_win.obj pulled in for symbol "class std::basic_string<wchar_t,struct std::char_traits<wchar_t>,class std::allocator<wchar_t> > __cdecl content::AccessibilityEventToString(int)" (?AccessibilityEventToString@content@@YA?AV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@H@Z" by
browser_accessibility_event_win.obj
browser_accessibility_event_win.obj pulled in for symbol "public: static class content::BrowserAccessibilityEvent * __cdecl content::BrowserAccessibilityEvent::Create(enum content::BrowserAccessibilityEvent::Source,enum ui::AXEvent,class content::BrowserAccessibility const *)" (?Create@BrowserAccessibilityEvent@content@@SAPEAV12@W4Source@12@W4AXEvent@ui@@PEBVBrowserAccessibility@2@@Z" by
browser_accessibility_manager.obj
browser_accessibility_manager.obj pulled in for symbol "public: void __cdecl content::BrowserAccessibilityManager::UserIsNavigatingAway(void)" (?UserIsNavigatingAway@BrowserAccessibilityManager@content@@QEAAXXZ" by
web_contents_impl.obj
web_contents_impl.obj pulled in for symbol "public: class content::BrowserPluginGuest * __cdecl content::WebContentsImpl::GetBrowserPluginGuest(void)const " (?GetBrowserPluginGuest@WebContentsImpl@content@@QEBAPEAVBrowserPluginGuest@2@XZ" by
Command-line obj file: guest_mode.obj
So now accessibility_tree_formatter_utils_win is still being pulled in, but indirectly. "Command-line obj file" means that guest_mode.obj was specified directly - probably another source_set. In this case source_set("browser_sources"). So I changed that to a static_library and tried again:
>python tools\win\linker_verbose_tracking.py chrome_child_02_browser_sources_to_static_library.txt accessibility_tree_formatter_utils_win
Database loaded - 12155 xrefs found
accessibility_tree_formatter_utils_win.obj pulled in for symbol "class std::basic_string<wchar_t,struct std::char_traits<wchar_t>,class std::allocator<wchar_t> > __cdecl content::AccessibilityEventToString(int)" (?AccessibilityEventToString@content@@YA?AV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@H@Z" by
browser_accessibility_event_win.obj
browser_accessibility_event_win.obj pulled in for symbol "public: static class content::BrowserAccessibilityEvent * __cdecl content::BrowserAccessibilityEvent::Create(enum content::BrowserAccessibilityEvent::Source,enum ui::AXEvent,class content::BrowserAccessibility const *)" (?Create@BrowserAccessibilityEvent@content@@SAPEAV12@W4Source@12@W4AXEvent@ui@@PEBVBrowserAccessibility@2@@Z" by
browser_accessibility_manager.obj
browser_accessibility_manager.obj pulled in for symbol "public: void __cdecl content::BrowserAccessibilityManager::UserIsNavigatingAway(void)" (?UserIsNavigatingAway@BrowserAccessibilityManager@content@@QEAAXXZ" by
web_contents_impl.obj
web_contents_impl.obj pulled in for symbol "public: static class content::WebContents * __cdecl content::WebContents::Create(struct content::WebContents::CreateParams const &)" (?Create@WebContents@content@@SAPEAV12@AEBUCreateParams@12@@Z" by
headless_web_contents_impl.obj
headless_web_contents_impl.obj pulled in for symbol "public: __cdecl headless::HeadlessWebContents::Builder::~Builder(void)" (??1Builder@HeadlessWebContents@headless@@QEAA@XZ" by
headless_shell.obj
headless_shell.obj pulled in for symbol "int __cdecl headless::HeadlessShellMain(struct content::ContentMainParams const &)" (?HeadlessShellMain@headless@@YAHAEBUContentMainParams@content@@@Z" by
Command-line obj file: chrome_main.obj
And that seems to explain things, or at least some things. headless_shell.obj pulls in headless_web_contents_impl.obj which pulls in web_contents_impl.obj which pulls in browser_accessibility_manager.obj which pulls in browser_accessibility_event_win.obj which pulls in accessibility_tree_formatter_utils_win.obj.
I uploaded my changes plus your CL to crrev.com/2858113005.
My build settings are:
is_component_build = false
is_debug = false
target_cpu = "x64"
Note that the two static_library changes shrink chrome_child.dll by about 5 KB. Not enough to be interesting, but sometimes multiple changes are needed in order to finally trigger the big savings.
,
May 4 2017
BTW, the reason the script was finding nothing for the author of the CL is because their verbose output is UTF16 and my script assumes ASCII or utf-8 or something like that. I'll land some changes to improve the diagnostics and maybe handle that.
,
May 5 2017
Thanks for the pointers Bruce! I investigated the next symbol: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.2.txt indexed_db.mojom Database loaded - 12153 xrefs found No references to indexed_db.mojom.obj found. Directly specified in sources or a source_set? Which gets referenced in https://cs.chromium.org/chromium/src/out/Debug/gen/content/common/indexed_db/indexed_db.mojom.cc A wild guess is that is generated in a here: https://cs.chromium.org/chromium/src/content/common/BUILD.gn?rcl=84b8a5f1c2f1c62aadd4c943f3f093860cdc470d&l=576 mojom("mojo_bindings") => Checking this doc: https://docs.google.com/document/d/1pLsOZ2PQ_FNZHx_U94ZJprDfGLQUgUkCssCoyC9TVVY/edit#heading=h.pl8mmvgw8xx7 It suggests that mojom targets are actually source sets, so that might be a problem for this build. I'll drop an email to the mojo team to see if that's the case and if it can be solved. If not I guess I'll move on to the next symbol
,
May 5 2017
For reference, this is the mojo thread: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/rlfodrnJA-A
,
May 5 2017
Another symbol: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.txt webui_resources_map Database loaded - 12242 xrefs found No references to webui_resources_map.obj found. Directly specified in sources or a source_set? webui_resource_map is declared here as a grit resource: https://cs.chromium.org/chromium/src/ui/resources/BUILD.gn?rcl=a11f72c868235d4673cd96c7eea24ad9e7eb4b55&l=36 This I assume is part of webui_resources.pak which is repacked in headless: https://cs.chromium.org/chromium/src/headless/BUILD.gn?rcl=04bee6a1d135e6665c04d1e748cf7baeb6769788&l=28 And added in headless component. However I have commented out adding the pak dependency here https://cs.chromium.org/chromium/src/headless/BUILD.gn?rcl=04bee6a1d135e6665c04d1e748cf7baeb6769788&l=360 and the object is still pulled for some reason
,
May 5 2017
Continuing Bruce investigation on #c37: The issue here is that headless_shell pulls a dependency to web_contents_impl this dependency doesn't exist in the current child_dll: python .\tools\win\linker_verbose_tracking.py C:\tmp\chrome.child.link.release.txt web_contents_impl Database loaded - 11504 xrefs found No references to web_contents_impl.obj found. Directly specified in sources or a source_set? Note that it is mentioned directly, but not pulled indirectly like headless library. My question are: - Do we really need to have a dependency to WebContents in headless child, given that this is not necessary for standard Chrome. - Do removing the indirect dependency result in a (significant) decrease in size in DLL. The problem is that this doesn't seem to be straightforward with the current headless implementation, and it might need significant regfractoring work.
,
May 5 2017
Re: mojo, there's no way to use the headless mojo bindings in runtime mode, so we could add a build time check that avoids compiling them in. I'm not sure why the linker doesn't realize this but maybe it isn't trivial. Only the browser part of headless should be using WebContents -- it's not even available in the renderer. Do you know which object is pulling it into the child dll? Are we just splitting headless/headless_child in the wrong place?
,
May 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3647b59ef9a6cf01783c7cf7b5ea83eab3296007 commit 3647b59ef9a6cf01783c7cf7b5ea83eab3296007 Author: brucedawson <brucedawson@chromium.org> Date: Fri May 05 22:40:30 2017 Improve linker_verbose_tracking.py linker_verbose_tracking.py is used to analyze the verbose output from the VC++ linker to help understand binary-size regressions. However some of its messages are clear only to the original author, and it completely fails on the utf-16 files created when redirecting the linker output from powershell. This change updates the script to handle both utf-16 and utf-8 files, and improves the messages printed in a couple of cases. R=dvallet@chromium.org BUG= 714841 Review-Url: https://codereview.chromium.org/2863843003 Cr-Commit-Position: refs/heads/master@{#469783} [modify] https://crrev.com/3647b59ef9a6cf01783c7cf7b5ea83eab3296007/tools/win/linker_verbose_tracking.py
,
May 8 2017
Thanks for your thoughts Bruce & Sami! So I continued looking into why is WebContents is pulled in and if this is the main cause of grief for the size regression. So, it turns out that if I put the code in HeadlessShell::OnStart and HeadlessContentMainDelegate::RunProcess behind an #if !defined(CHROME_MULTIPLE_DLL_CHILD) block. I can verify that WebContentsImpl is not pulled in indirectly anymore and the dll size goes from 75MB to 67MB in component build, which I think it is almost all the size regression problem. I'll work tomorrow on a working patch and verifying on an official build, but I'd say we're close to the solution.
,
May 10 2017
SO I got a working patch and did an oficial build. This is the current difference in size: ls .\out\Release\chrome_child.dll -a---- 5/10/2017 3:32 PM 71081472 chrome_child.dll ls .\out\ReleaseHeadless\chrome_child.dll -a---- 5/10/2017 12:53 PM 71152640 chrome_child.dll ls .\out\Release\chrome.dll -a---- 5/10/2017 3:21 PM 50447360 chrome.dll ls .\out\ReleaseHeadless\chrome.dll -a---- 5/10/2017 12:25 PM 50616832 chrome.dll So that's roughly ~70KB increase in chrome_child_dll and ~165KB increase in chrome_dll. This seems in line with the ~150KB increase in Linux headless. I'll add the relevant people to the reland cl to double check.
,
May 12 2017
those sizes seem a bit off, I get from examining an official build, in this case, 60.0.3097.0: 32-bit chrome.dll - 34,992,472 chrome_child.dll - 47,093,080 64-bit chrome.dll - 49,562,968 chrome_child.dll - 61,548,888 is the 70kb and 165kb increase on your sizes - which would make it a 0.1% and 0.3% increase, or on the sizes from the official builders, which would make it a 0.15% and 0.48% increase? 0.5% is quite high, but I suppose within the realms of acceptable (I'm not the final decider on this anyway, just providing more data).
,
May 12 2017
> those sizes seem a bit off, I get from examining an official build, in this case, 60.0.3097.0: wfh: when you say official, do you really mean PGO? This situation where the builds we ship (PGO) don't match the official build config is seriously confusing. ( crbug.com/673025 ) dvallet: How did you configure your official build?
,
May 12 2017
>wfh: when you say official, do you really mean PGO? yes, I mean the builds from the official build archive, which are PGO... yes I agree it's quite confusing, I don't think it's possible to actually self-generate the same type of builds that we ship to users - I starred that bug :) FWIW - I don't think that a 0.5% size regression necessarily blocks this CL from landing - I think we can land and then examine the real size diffs on the build archive as well...
,
May 12 2017
My build config is: is_chrome_branded = true is_debug = false is_official_build = true +1 for landing and checking the real size digg
,
May 13 2017
,
May 13 2017
Looks like the real size increase for chrome_child.dll was indeed 0.1% for chrome_child and 0.3% for chrome.dll. There's also a ~0.15% increase in mini_installer.exe https://chromeperf.appspot.com/report?sid=fcb4b715aa409ce28f65cb5c3afed7a56c89cb910a7a0b81e41ad0c785892b7d&start_rev=471498&end_rev=471573 This hasn't triggered any alert.
,
May 13 2017
I don't think expressing size deltas as percentages is very useful; chrome is large enough that even fairly large size increases look like a small percentage. What's headless -- which capabilities does it add, and what code does it pull in that isn't in chrome already? I'm guessing it went through launch review, and this size increase is in line with the estimate that was approved there?
,
May 13 2017
Sorry, I'll add more context: We are adding a headless run-time environment for Chrome, running with the flag --headless. This pulls a new library from src/headless Overall launch bug: https://bugs.chromium.org/p/chromium/issues/detail?id=705916 Bug for --headless flag in Linux: https://bugs.chromium.org/p/chromium/issues/detail?id=612904. You can see in this design doc that the impact in linux was ~150KB, but up to ~600KB was deemed accectable: https://docs.google.com/document/d/1aIJUzQr3eougZQp90bp4mqGr5gY6hdUice8UPa-Ys90/edit#heading=h.qxqfzv2lj12s The absolute increase is 160KB for chrome.dll and 70KB for chrome_child.dll
,
May 15 2017
If nobody has strong feelings against it I'm inclined to close this bug. Thanks everybody for their help!
,
May 15 2017
#55 - perhaps you should file a new issue for the "(and nobody noticed?)" part?
,
May 15 2017
Let's close this bug. I'll file a new bug for the "nobody noticed" part.
,
May 15 2017
Filed bug 722400 for monitoring. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by h...@chromium.org
, Apr 24 2017