New issue
Advanced search Search tips

Issue 883902 link

Starred by 1 user
Status: Fixed
Owner:
fdoray@chromium.org
Closed: Nov 9
Cc:
agrieve@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

20kb regression in resource_sizes (MonochromePublic.apk) at 590667:590667

Project Member Reported by agrieve@chromium.org, Sep 13 
Caused by “Base: Mark TimeDelta::In...() methods as constexpr.”

Commit: 4d9714247f617bf9e1cf95f735148f5e598a9529

Link to size graph: https://chromeperf.appspot.com/report?sid=bb23072657e2d7ca892a1c3fa4643b1ee29b3a0a44d0732adda87168e89c0380&num_points=10&rev=590667

Debugging size regressions is documented at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/apk_size_regressions.md#Debugging-Apk-Size-Increase

Based on the graph, increase is from a lot of functions getting slightly bigger.

Here's the link to the binary size trybot for the review:
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-binary-size/52511

It looks like this increase was probably unexpected or might be avoidable.
Please have a look and either:

Close as “Won't Fix” with a short justification, or
Land a revert / fix-up.
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Sep 13 

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

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


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

Android Builder Perf
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Sep 13 

Assigning to fdoray@chromium.org because this is the only CL in range:
Base: Mark TimeDelta::In...() methods as constexpr.

This will allow use of these methods in constexpr expressions such as
https://chromium-review.googlesource.com/c/chromium/src/+/1195852/4/chrome/browser/resource_coordinator/tab_manager_features.h#118

Change-Id: I4407ac72846e7c826e5cf3d920cda5f4b5d5071c
Reviewed-on: https://chromium-review.googlesource.com/1220446
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590667}

Comment 3 by fdoray@google.com, Sep 14

Status: Started (was: Assigned)
Started investigating whether using NOINLINE could prevent the regression: https://chromium-review.googlesource.com/c/chromium/src/+/1226818
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 8 

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

commit 874f09c68a213663767a7c25d8d4beb4726c45f5
Author: François Doray <fdoray@chromium.org>
Date: Thu Nov 08 15:11:07 2018

Revert "Base: Mark TimeDelta::In...() methods as constexpr."

This reverts commit 4d9714247f617bf9e1cf95f735148f5e598a9529.

Reason for revert: Caused a binary size regression on Android.  https://crbug.com/883902 

TBR=alemate@chromium.org,chili@chromium.org,kouhei@chromium.org

Bug:  883902 

Original change's description:
> Base: Mark TimeDelta::In...() methods as constexpr.
>
> This will allow use of these methods in constexpr expressions such as
> https://chromium-review.googlesource.com/c/chromium/src/+/1195852/4/chrome/browser/resource_coordinator/tab_manager_features.h#118
>
> Change-Id: I4407ac72846e7c826e5cf3d920cda5f4b5d5071c
> Reviewed-on: https://chromium-review.googlesource.com/1220446
> Commit-Queue: François Doray <fdoray@chromium.org>
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#590667}

TBR=fdoray@chromium.org,mark@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: Ib3b8ead36933549c238622ea2b6641dc427f65ad
Reviewed-on: https://chromium-review.googlesource.com/c/1226098
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606467}
[modify] https://crrev.com/874f09c68a213663767a7c25d8d4beb4726c45f5/base/time/time.cc
[modify] https://crrev.com/874f09c68a213663767a7c25d8d4beb4726c45f5/base/time/time.h
[modify] https://crrev.com/874f09c68a213663767a7c25d8d4beb4726c45f5/chrome/browser/chromeos/power/auto_screen_brightness/adapter.h
[modify] https://crrev.com/874f09c68a213663767a7c25d8d4beb4726c45f5/chrome/browser/resource_coordinator/tab_manager_features.h
[modify] https://crrev.com/874f09c68a213663767a7c25d8d4beb4726c45f5/components/offline_pages/core/background/request_coordinator.cc
[modify] https://crrev.com/874f09c68a213663767a7c25d8d4beb4726c45f5/content/browser/web_package/signed_exchange_signature_verifier.cc

Comment 5 by fdoray@chromium.org, Nov 9

Status: Fixed (was: Started)
These graphs confirm that the revert fixed the binary size regression: https://chromeperf.appspot.com/report?sid=bb23072657e2d7ca892a1c3fa4643b1ee29b3a0a44d0732adda87168e89c0380&num_points=10&rev=606467
►

Sign in to add a comment