Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1644773005:
[Android] Change resource_sizes.py to get static initializers the same as size.py (Closed)
|
Created:
4 years, 10 months ago by rnephew (Reviews Here) Modified:
4 years, 10 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Change resource_sizes.py to get static initializers the same as size.py
BUG=546012
Committed: https://crrev.com/b5fe09765ff18e8f17e1b4af4a4dc9f6e7599ce9
Cr-Commit-Position: refs/heads/master@{#374679}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Messages
Total messages: 17 (4 generated)
The new method for getting static initializers comes from size.py in the infra repo.
https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... build/android/resource_sizes.py:84: dump_initializers = os.path.join(host_paths.DIR_SOURCE_ROOT, 'tools', 'linux', nit: maybe this can be a module-level constant? https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... build/android/resource_sizes.py:318: static_initializers = GetStaticInitializers(so_with_symbols_path) Asking mostly out of ignorance, but could si_count != len(static_initializers)? Also, why do we GetStaticInitializers? It seems that we only print them to stdout, but do nothing else with them.
https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... build/android/resource_sizes.py:84: dump_initializers = os.path.join(host_paths.DIR_SOURCE_ROOT, 'tools', 'linux', On 2016/02/01 10:23:59, perezju wrote: > nit: maybe this can be a module-level constant? Done. https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... build/android/resource_sizes.py:318: static_initializers = GetStaticInitializers(so_with_symbols_path) On 2016/02/01 10:23:59, perezju wrote: > Asking mostly out of ignorance, but could si_count != len(static_initializers)? > > Also, why do we GetStaticInitializers? It seems that we only print them to > stdout, but do nothing else with thThe dump-static-initializers.py script that sizes.py use is currently broken with android builds. If you look at the previous CL, there is more context for this: https://codereview.chromium.org/1445633002/
lgtm
https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... File build/android/resource_sizes.py (right): https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... build/android/resource_sizes.py:318: static_initializers = GetStaticInitializers(so_with_symbols_path) On 2016/02/01 15:46:19, rnephew1 wrote: > On 2016/02/01 10:23:59, perezju wrote: > > Asking mostly out of ignorance, but could si_count != > len(static_initializers)? > > > > Also, why do we GetStaticInitializers? It seems that we only print them to > > stdout, but do nothing else with thThe dump-static-initializers.py script that > sizes.py use is currently broken with android builds. If you look at the > previous CL, there is more context for this: > https://codereview.chromium.org/1445633002/ I had a look but I'm still confused: 1. is si_count == len(static_initializers) always true? if not, why not? 2. why do we need to compute the list of static_initializers if they are not reported as perf results?
On 2016/02/03 11:09:02, perezju wrote: > https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... > File build/android/resource_sizes.py (right): > > https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... > build/android/resource_sizes.py:318: static_initializers = > GetStaticInitializers(so_with_symbols_path) > On 2016/02/01 15:46:19, rnephew1 wrote: > > On 2016/02/01 10:23:59, perezju wrote: > > > Asking mostly out of ignorance, but could si_count != > > len(static_initializers)? > > > > > > Also, why do we GetStaticInitializers? It seems that we only print them to > > > stdout, but do nothing else with thThe dump-static-initializers.py script > that > > sizes.py use is currently broken with android builds. If you look at the > > previous CL, there is more context for this: > > https://codereview.chromium.org/1445633002/ > > I had a look but I'm still confused: > > 1. is si_count == len(static_initializers) always true? if not, why not? It is not true for android right now, because si_count just reads from the output of readelf on the so file, and reports how many SIs there are based off of word size. static_initializers attempts to use dump-static-initializers.py to dump all the SIs, but currently does not work for all architectures (arm included). > 2. why do we need to compute the list of static_initializers if they are not > reported as perf results? In an ideal world we could just use len(static_initializers) but until dump-static-initializers works on arm we cannot use it to calculate how many static initializers there are.
On 2016/02/04 17:09:27, rnephew1 wrote: > On 2016/02/03 11:09:02, perezju wrote: > > > https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... > > File build/android/resource_sizes.py (right): > > > > > https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... > > build/android/resource_sizes.py:318: static_initializers = > > GetStaticInitializers(so_with_symbols_path) > > On 2016/02/01 15:46:19, rnephew1 wrote: > > > On 2016/02/01 10:23:59, perezju wrote: > > > > Asking mostly out of ignorance, but could si_count != > > > len(static_initializers)? > > > > > > > > Also, why do we GetStaticInitializers? It seems that we only print them to > > > > stdout, but do nothing else with thThe dump-static-initializers.py script > > that > > > sizes.py use is currently broken with android builds. If you look at the > > > previous CL, there is more context for this: > > > https://codereview.chromium.org/1445633002/ > > > > I had a look but I'm still confused: > > > > 1. is si_count == len(static_initializers) always true? if not, why not? > It is not true for android right now, because si_count just reads from the > output of readelf on the so file, and reports how many SIs there are based off > of word size. > static_initializers attempts to use dump-static-initializers.py to dump all the > SIs, but currently does not work for all architectures (arm included). > > > 2. why do we need to compute the list of static_initializers if they are not > > reported as perf results? > In an ideal world we could just use len(static_initializers) but until > dump-static-initializers works on arm we cannot use it to calculate how many > static initializers there are. Ok, could you please: add that explanation using comments on the code; file a bug to get this fixed; and on the output when you print the list of static_analyzers (which presumably ends up in the stdio logs) _also_ print the number of SIs found, together with a warning about missing SIs if the numbers do not match?
On 2016/02/05 10:35:50, perezju wrote: > On 2016/02/04 17:09:27, rnephew1 wrote: > > On 2016/02/03 11:09:02, perezju wrote: > > > > > > https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... > > > File build/android/resource_sizes.py (right): > > > > > > > > > https://codereview.chromium.org/1644773005/diff/1/build/android/resource_size... > > > build/android/resource_sizes.py:318: static_initializers = > > > GetStaticInitializers(so_with_symbols_path) > > > On 2016/02/01 15:46:19, rnephew1 wrote: > > > > On 2016/02/01 10:23:59, perezju wrote: > > > > > Asking mostly out of ignorance, but could si_count != > > > > len(static_initializers)? > > > > > > > > > > Also, why do we GetStaticInitializers? It seems that we only print them > to > > > > > stdout, but do nothing else with thThe dump-static-initializers.py > script > > > that > > > > sizes.py use is currently broken with android builds. If you look at the > > > > previous CL, there is more context for this: > > > > https://codereview.chromium.org/1445633002/ > > > > > > I had a look but I'm still confused: > > > > > > 1. is si_count == len(static_initializers) always true? if not, why not? > > It is not true for android right now, because si_count just reads from the > > output of readelf on the so file, and reports how many SIs there are based off > > of word size. > > static_initializers attempts to use dump-static-initializers.py to dump all > the > > SIs, but currently does not work for all architectures (arm included). > > > > > 2. why do we need to compute the list of static_initializers if they are not > > > reported as perf results? > > In an ideal world we could just use len(static_initializers) but until > > dump-static-initializers works on arm we cannot use it to calculate how many > > static initializers there are. > > Ok, could you please: add that explanation using comments on the code; file a > bug to get this fixed; and on the output when you print the list of > static_analyzers (which presumably ends up in the stdio logs) _also_ print the > number of SIs found, together with a warning about missing SIs if the numbers do > not match? Done. crbug.com/585588
lgtm w/nits https://codereview.chromium.org/1644773005/diff/40001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/1644773005/diff/40001/build/android/resource_... build/android/resource_sizes.py:317: # static initializers. This does not work on all archs (particularly arm). nit: add a TODO with a link to the bug to get this fixed. https://codereview.chromium.org/1644773005/diff/40001/build/android/resource_... build/android/resource_sizes.py:323: print '%s Files with static initializers:' % si_count nit: I would rephrase these as: sizes match: - 'Found %d files with static initializers:' % si_count sizes do not match: - 'There are %d files with static initializers, but dump-static-initializers found %d:' % (si_count, len(static_intializers))
https://codereview.chromium.org/1644773005/diff/40001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/1644773005/diff/40001/build/android/resource_... build/android/resource_sizes.py:317: # static initializers. This does not work on all archs (particularly arm). On 2016/02/10 09:56:17, perezju wrote: > nit: add a TODO with a link to the bug to get this fixed. Done. https://codereview.chromium.org/1644773005/diff/40001/build/android/resource_... build/android/resource_sizes.py:323: print '%s Files with static initializers:' % si_count On 2016/02/10 09:56:17, perezju wrote: > nit: I would rephrase these as: > > sizes match: > - 'Found %d files with static initializers:' % si_count > > sizes do not match: > - 'There are %d files with static initializers, but dump-static-initializers > found %d:' % (si_count, len(static_intializers)) Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/1644773005/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1644773005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1644773005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Change resource_sizes.py to get static initializers the same as size.py BUG=546012 ========== to ========== [Android] Change resource_sizes.py to get static initializers the same as size.py BUG=546012 Committed: https://crrev.com/b5fe09765ff18e8f17e1b4af4a4dc9f6e7599ce9 Cr-Commit-Position: refs/heads/master@{#374679} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b5fe09765ff18e8f17e1b4af4a4dc9f6e7599ce9 Cr-Commit-Position: refs/heads/master@{#374679} |
