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

Issue 787766 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 27
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

11.8% regression in blink_perf.svg at 517978:518030

Project Member Reported by hjd@google.com, Nov 22 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Nov 22 2017

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

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


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

android-webview-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Nov 22 2017

Cc: ikilpatrick@chromium.org
Owner: ikilpatrick@chromium.org
Status: Assigned (was: Untriaged)

=== Auto-CCing suspected CL author ikilpatrick@chromium.org ===

Hi ikilpatrick@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Ian Kilpatrick
  Commit : 98b46470f307a68c8183d628237d7b361af96244
  Date   : Tue Nov 21 01:22:24 2017
  Subject: Add secure_context_mode_ to CSSParserContext.

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : blink_perf.svg
  Metric       : Bamboo/Bamboo
  Change       : 10.52% | 1454.81766667 -> 1607.86816667

Revision             Result                  N
chromium@517977      1454.82 +- 16.7504      6      good
chromium@518004      1460.36 +- 16.0688      6      good
chromium@518011      1456.55 +- 15.2178      6      good
chromium@518014      1450.75 +- 11.3951      6      good
chromium@518015      1449.48 +- 13.2347      6      good
chromium@518016      1634.91 +- 19.5079      6      bad       <--
chromium@518017      1632.9 +- 14.1436       6      bad
chromium@518030      1607.87 +- 14.7934      6      bad

Please refer to the following doc on diagnosing blink_perf regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/speed/benchmark_harnesses/blink_perf.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.svg

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8962230298193055440


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Nov 23 2017

 Issue 787767  has been merged into this issue.
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Nov 23 2017

 Issue 787769  has been merged into this issue.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Nov 24 2017

 Issue 787855  has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Nov 24 2017

 Issue 788327  has been merged into this issue.
Project Member

Comment 8 by 42576172...@developer.gserviceaccount.com, Nov 24 2017

 Issue 788363  has been merged into this issue.
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Nov 24 2017

 Issue 788378  has been merged into this issue.
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Nov 24 2017

 Issue 788348  has been merged into this issue.
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Nov 25 2017

 Issue 788380  has been merged into this issue.
Cc: haraken@chromium.org yosin@chromium.org meade@chromium.org
ikilpatrick: Can you please take a look? There seem to be a lot of performance regressions caused by this CL. Adding reviewers in case they have ideas.
Sorry - this slipped off my radar.

The context for this change is:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/Jex3idOld48/VUYks4U4BAAJ

Basically as we decided for now to ship the CSS Paint API only on secure contexts, we needed to wire an additional bit (secure_context_mode_) into the CSS Parser.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parser/CSSParserContext.h?q=cssparsercontext&sq=package:chromium&l=132

This is so that things like "background: paint(foo)" fails to parse on insecure contexts.

Checking this bit itself is cheap, and only happens in a few cases (at the moment) but the performance regression come in as we need to pass in the right the document (or more generally the execution context) to determine if its secure or not. I.e.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSGroupingRule.cpp?type=cs&l=69

The regression is coming from the bindings code, which now has to look up what the correct execution context is, to pass into the function. E.g.
https://chromium-review.googlesource.com/c/chromium/src/+/759112/16/third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl

The IsSecureContext check on ExecutionContext itself is _now_ cheap (it wasn't when this patch first landed, hence the improvement after a few days).
This was fixed in: 
https://chromium.googlesource.com/chromium/src/+/9371bdbfccefb2e3eead2c017ef2c9c924dbc06c

@haraken might have thoughts on how we can get rid of some overhead, i.e. lazily looking up an execution context from FunctionCallbackInfo??? But this seems like a bad maintenance burden.
Any updates here?

Sign in to add a comment