New issue
Advanced search Search tips

Issue 739793 link

Starred by 3 users
Status: Fixed
Owner:
karandeepb@chromium.org
Closed: Jul 2017
Cc:
agrieve@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Static initializer added (MonochromePublic.apk) at 484349:484349

Project Member Reported by agrieve@chromium.org, Jul 6 2017 
Looks like you're the first to trigger the static initializer warning since it was introduced on Android! 🎁

Here's the graph:
https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg3tbMrgsM

We hope to add a trybot for this this quarter, but for now, I'd suggest reverting and then running:

build/android/resource_sizes.py out-gn/Debug/apks/Chrome.apk --chromium-output-directory out-gn/Debug --dump-static-initializers
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Jul 6 2017 

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=739793

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


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

Android Builder

Comment 2 by karandeepb@chromium.org, Jul 6 2017 

agrieve@: I can't seem to run the command build/android/resource_sizes.py out-gn/Debug/apks/Chrome.apk --chromium-output-directory out-gn/Debug --dump-static-initializers

Should I replace out-gn/Debug with out/Default (which I use as my build dir)? Even after doing so, I get "No such file or directory" for out/Default/build_vars.txt. Is this command supported on all platforms?

Comment 3 by agrieve@chromium.org, Jul 6 2017 

Hey! No, this is android-only. There are actually trybots for SIs on Linux already, so likely this SI is android-only.

Certainly need to use your own output directory. I think you make also need to build release (or at least a non-component build)

Comment 4 by karandeepb@chromium.org, Jul 6 2017 

Oh ok. I'll issue a fix to remove the static initializers and check the Performance Dashboard.

Comment 5 by karandeepb@chromium.org, Jul 6 2017 

Another question: static function-local variables are ok right?

Comment 6 by agrieve@chromium.org, Jul 7 2017 

The main reason static initializers are "bad" is because they run before main(). So, function-local statics are fine, as they are not initialized until their first occurrence.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 10 2017 

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

commit b8d0d56379501950d869c5bb253aa41c15444df3
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Mon Jul 10 19:39:25 2017

UrlPatternIndex: Remove static initializers added in r484349.

r484349 added a couple of non-trivially destructible static variables, which are
prohibited by the Google style guide. This CL removes the same by replacing them
with static function-local variables which are leaked.

BUG= 739793 

Change-Id: I99a6193135092568b877990034b79faf76ae290d
Reviewed-on: https://chromium-review.googlesource.com/562520
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485350}
[modify] https://crrev.com/b8d0d56379501950d869c5bb253aa41c15444df3/components/url_pattern_index/url_pattern_index.cc

Comment 8 by karandeepb@chromium.org, Jul 11 2017

Status: Fixed (was: Assigned)
This is fixed now as can be seen from https://chromeperf.appspot.com/group_report?sid=7947c7be2077c57690937f2e37ed80e78797d2feaf2385b7871bd39229ab026a
►

Sign in to add a comment