New issue
Advanced search Search tips

Issue 869997 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 22
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 871710



Sign in to add a comment

thinLTO makes object files not readable by the orderfile generator

Project Member Reported by mattcary@chromium.org, Aug 1

Issue description

ThinLTO produce bitcode files so that it can do link-time optimization. This is not playing nicely with the orderfile generator which relies on objdump to extract symbol names from object files.

Orderfile generation is still working. I believe this only affects the patching step of the orderfile, which may be second-order. 

The immediate symptom is the following error messages during orderfile generation (this message taken from the buildbot):
/b/build/slave/orderfile-clankium/build/src/third_party/android_ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-objdump: /b/build/slave/orderfile-clankium/build/src/arm_instrumented_out/Release/obj/components/visitedlink/common/common/visitedlink_common.o: File format not recognized  

Looking into the composition of the object files (this is from a private run of the new system-health based orderfile).

$ find obj/ -name \*.o | xargs file |awk '{ print $2 $3 $4 }' | sort | uniq -c                                                
   2451 ELF32-bitLSB                                              
  20736 LLVMIRbitcode

Note that was the instrumented code, the uninstrumented version is similar:
/clank/code/src/out/Release/arm_uninstrumented_out/Release$ find obj/ -name \*.o | xargs file | awk '{ print $2 $3 $4 }' | sort | uniq -c
   2437 ELF32-bitLSB                                                  
  20599 LLVMIRbitcode 
 
Further thoughts: this isn't just affecting patching, this prevents us from mapping symbol names to the section names that need to go in the orderfile. That's a first-order problem :(
The orderfile does not seem to have changed much since LTO was turned on, which while it means this is not dire, is very confusing.

LTO broke the orderfile for an unrelated reason for several days (crbug.com/867878), so when LTO was turned on is clearly visible in the log, with b8320cfd17 being the first post-LTO CL:

/clank/code/src/clank$ git lg orderfiles/orderfile.arm.out.sha1 | head
786d5981d8 - (Thu Jul 26 06:38:50 2018) Update Orderfile. <clank-autoroller>
abfac0e5f1 - (Thu Jul 26 03:38:13 2018) Update Orderfile. <clank-autoroller>
b8320cfd17 - (Thu Jul 26 02:26:40 2018) Update Orderfile. <clank-autoroller>
a79374d686 - (Wed Jul 18 17:12:04 2018) Update Orderfile. <clank-autoroller>
fdbeca77e3 - (Wed Jul 18 13:40:18 2018) Update Orderfile. <clank-autoroller>
bdf6ddc146 - (Wed Jul 18 11:04:03 2018) Update Orderfile. <clank-autoroller>
e5eafcf94f - (Wed Jul 18 07:07:19 2018) Update Orderfile. <clank-autoroller>

/clank/code/src$ ./tools/cygprofile/compare_orderfiles.py --from-commit=b8320cfd17
Symbols count:
	first:	233788
	second:	233504
New symbols = 4533
Removed symbols = 4817
Average fractional distance = 1.23%

This is not much bigger than previous changes:
/clank/code/src$ ./tools/cygprofile/compare_orderfiles.py --from-commit=bdf6ddc146
Symbols count:
	first:	233209
	second:	233189
New symbols = 174
Removed symbols = 194
Average fractional distance = 0.43%


I changed the orderfile to only work with symbols, which avoids scanning object files for section names. See crrev.com/c/1162221. I also updated the orderfile generator to use a fixed set of profile files, to avoid changes in symbols seen during instrumentation (crrev.com/c/1162179).

From a fixed profile, the current generation produces 67850 symbols. The orderfile without sections, which avoids the thinLTO problem, has 127816 symbols. That's expected. We would further expect that the section-based orderfile to be a subset of the other one, but that's not true; there are 10173 symbols in the section-based orderfile that aren't found in the other one.

There is trickiness around the sections for static constructors and destructors, however that does not account for the change as ~8000 of the unexpected symbols are not static constructors or destructors (in the sense of not matching (C[123]|D[012])E).

Perhaps the orderfile affects function inlining, or function inlining is surprisingly variable. This doesn't seem very plausible.

Given the lack of actionable data, my inclination is to remove the section processing and move forward, even if we don't understand why ThinLTO hasn't totally broken the orderfile.
Cc: mattcary@chromium.org
 Issue 832131  has been merged into this issue.
Blocking: 871710
This seems to actually be a problem as evidenced by crbug.com/871710 as well as the (google-only) %-resident on startup timeline:

https://uma.googleplex.com/p/chrome/timeline_v2?sid=d2faf3b77c8b1db49f4d1a43f119ad86

This means that there has been something wrong with the section-based orderfile. CLs to move to symbol-only are in process of being reviewed.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 10

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

commit 14195b4f7408cb672b372126465b8583eb4e1ed9
Author: Matthew Cary <mattcary@chromium.org>
Date: Fri Aug 10 14:59:57 2018

Orderfile: direct verification of symbol ordering.

Bug:  869997 
Change-Id: I867cb67d5fc3e4e4a7b3343053c6f38123c3e776
Reviewed-on: https://chromium-review.googlesource.com/1162154
Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582163}
[modify] https://crrev.com/14195b4f7408cb672b372126465b8583eb4e1ed9/tools/cygprofile/check_orderfile.py
[modify] https://crrev.com/14195b4f7408cb672b372126465b8583eb4e1ed9/tools/cygprofile/check_orderfile_unittest.py

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 10

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

commit 69e9e42e2bd486447c5da5c462e636a85af3c7f4
Author: Matthew Cary <mattcary@chromium.org>
Date: Fri Aug 10 18:30:06 2018

Orderfile: options to save and reuse profile data.

Adds --pregenerated-profiles and --profile-save-dir to
orderfile_generator_backend.py. These allow for precise comparisons
between different versions of profile processing, that are not affected
by variations in the symbols seen during device profiling.

Bug:  869997 
Change-Id: I9dd734a77ea4a738e22f20f095e76879e71f3c0c
Reviewed-on: https://chromium-review.googlesource.com/1162179
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582258}
[modify] https://crrev.com/69e9e42e2bd486447c5da5c462e636a85af3c7f4/tools/cygprofile/orderfile_generator_backend.py
[modify] https://crrev.com/69e9e42e2bd486447c5da5c462e636a85af3c7f4/tools/cygprofile/profile_android_startup.py

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 10

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

commit 8b1416232ce40b42c76bf35e7ca601b842fbef9b
Author: Matthew Cary <mattcary@chromium.org>
Date: Fri Aug 10 19:12:22 2018

Orderfile: Only use symbols in orderfile generation

Removes gold-style section orderfile, as that is now not possible to
generate with ThinLTO in lld.

Bug:  869997 
Change-Id: I9d6970cf9c251cefdc69fbef6c87118fde844374
Reviewed-on: https://chromium-review.googlesource.com/1162221
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582282}
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/check_orderfile.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/check_orderfile_unittest.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/cyglog_to_orderfile.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/orderfile_generator_backend.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/patch_orderfile.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/patch_orderfile_unittest.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/process_profiles.py
[modify] https://crrev.com/8b1416232ce40b42c76bf35e7ca601b842fbef9b/tools/cygprofile/process_profiles_unittest.py

Status: Fixed (was: Started)
% resident on startup for prefetch has come back down to normal and remained stable, marking as fixed.

Sign in to add a comment