New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 838894 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Increase in tracked static initializer count (MonochromePublic.apk) at 555174:555174

Project Member Reported by cjgrant@google.com, May 2 2018

Issue description

An AR change, https://chromium-review.googlesource.com/1033580, appears to have added a static initializer (bumping the number from 5 to 6).

We should track down why, and see if we can avoid the increase.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=838894

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


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

Android Builder Perf
Assigning to vollick@chromium.org because this is the only CL in range:
Update build rules to support ARCore

Define enable_arcore and add some build rules to use it.

Bug: 833511
Change-Id: I716123c2a282d7d123883df81ebbc00a0a883be8
Reviewed-on: https://chromium-review.googlesource.com/1033580
Commit-Queue: Ian Vollick <vollick@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Eric Stevenson <estevenson@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555174}
Cc: -cjgrant@google.com estevenson@chromium.org jbudorick@chromium.org cjgrant@chromium.org agrieve@chromium.org
I've got a theory about what's happening here, and if I'm right, it suggests that my changes to the scripts in crrev.com/c/1033580 are bogus.

The reason that I'd updated the scripts is that the cq bots were failing to analyze the new lib I added via loadable_modules (it wasn't in the lib.unstripped folder like the other .so's).

My theory, though, is that we're starting the analysis via this code (https://cs.chromium.org/chromium/src/build/android/resource_sizes.py?rcl=63329b973b2b8b412f1b967c4e7ef742425aa438&l=735), rather than via assert_static_initializers and so the ignored_libs are not being set.

But if this is the case, and the code can actually analyze the new .so, then my rationale for ignoring seems wrong and we should revert that code.

If anyone knows about the metric and can confirm/refute my theory, I'd really appreciate your input/advice.
Cc: -estevenson@chromium.org vollick@chromium.org
Labels: OS-Android
Owner: estevenson@chromium.org
I'm not sure why this worked on the perf bot but I wasn't thinking about it when I suggested passing ignored_libs to AnalyzeStaticInitializers.

If this should actually work in assert_static_initializers.py then I'll fix it and update the expectations.

But regardless we don't need to revert your CL for adding a static initializer. Since the .so is a prebuilt we should just file a bug with the team that builds it.
The part that was failing previously uses dump-static-initializers.py to print out the names of symbols for static initializers. This requires the unstripped .so to be present - and for the arcore lib I'm assuming we just have a stripped release build checked in. 

The perf bot doesn't run this as it can be slow so that's why it didn't fail (actually seems to be quick now oddly enough).

I don't think it matters much either way since we mostly only care about static initializers in the library that loads when Chrome runs, i.e. libchrome/libmonochrome. Probably still worthwhile to track though - more so for when/if we add more libs in the future. 

I'll put up a CL for this today. 

Sign in to add a comment