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

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


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

chromium-rel-mac11-pro
chromium-rel-win7-gpu-intel
linux-release

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8964749861941777392

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

Hi yhirano@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 : Yutaka Hirano
  Commit : 8419e454a81a7e8a625c43caae69d674530f8298
  Date   : Tue Oct 24 12:05:22 2017
  Subject: Have ScriptForbiddenScope work in worker threads

Bisect Details
  Configuration: winx64intel_perf_bisect
  Benchmark    : speedometer
  Metric       : jQuery-TodoMVC/jQuery-TodoMVC
  Change       : 2.45% | 522.9605 -> 535.753277778

Revision             Result                  N
chromium@511067      522.961 +- 22.3967      9       good
chromium@511086      522.586 +- 9.00408      6       good
chromium@511096      522.763 +- 22.4465      9       good
chromium@511101      524.692 +- 23.6215      9       good
chromium@511103      528.456 +- 26.8581      14      good
chromium@511104      540.433 +- 25.8773      14      bad       <--
chromium@511105      541.346 +- 12.8382      9       bad
chromium@511143      535.753 +- 20.4         9       bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests speedometer

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

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


For feedback, file a bug with component Speed>Bisection

+haraken@, yukishiino@: Hm, it turns out that my expectation on  issue 777716 , "ScriptForbiddenScope methods are not so performance sensitive", is wrong. What do you think about the regression? The change is about correctness (may impact stability / security) and I don't want to revert it. Is it worthwhile to have a fast path for the main thread case?

The "fast path" I meant is already implemented in ThreadSpecific<T>::operator T*(). Now I don't have any idea how to mitigate the regression.

Yeah, a 3 - 4% regression on Speedometer is huge and not acceptable. Would you work on adding a fast path?

Also I was not thinking that ScriptForbiddenScope would be that performance-sensitive either. Do you know what ScriptForbiddenScope is causing the regression?

+Jeremy FYI

Given the magnitude of the regression and the unclarity around how to mitigate the regression, I think it might make sense to revert this change until it's all sorted out? Clearly the correctness issue must be addressed, but we cannot ship with this performance regression either, so until an overall suitable solution is found it would be nice to have consistency in the performance numbers.

Yeah, it would make more sense to revert it for now.

The change is proposed for a speculative fix for a stable-blocker crash so I would like to see if the change fixes the crash.

Just FYI, we, tzik@, yhirano@ and me, came up with an idea to speed up ScriptForbiddenScope, and I think that yhirano@ has already started the work.

 Issue 778608  has been merged into this issue.

I tried [1] but it didn't work well as we expected (see [2] in [3] and [4] in [5]).

1: https://chromium-review.googlesource.com/c/chromium/src/+/737401
2: https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/html-results/results-2017-10-26_02-52-49
3: https://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/2061
4: https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/html-results/results-2017-10-26_20-13-37
5: https://build.chromium.org/p/tryserver.chromium.perf/builders/win_x64_perf_bisect/builds/2064

https://chromium-review.googlesource.com/c/chromium/src/+/741202 worked well on my local environment.

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

commit 0d8c27e28d9fe88da1c86f4b8b1f284765e43c81
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Fri Oct 27 13:24:16 2017

Add a ScriptForbiddenScope optimization for the main thread use-case

This is a fix for some performance regressions introcued by
https://crrev.com/8419e454a81a7e8a625c43caae69d674530f8298.

This CL decreases the access cost for the counter on the main thread.

Bug:  777716 , 731019, 756406,  778270 
Change-Id: Ida8fd8e722a0cab97d74ca63d9e29f2da755674f
Reviewed-on: https://chromium-review.googlesource.com/741202
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512165}
[modify] https://crrev.com/0d8c27e28d9fe88da1c86f4b8b1f284765e43c81/third_party/WebKit/Source/platform/bindings/ScriptForbiddenScope.cpp
[modify] https://crrev.com/0d8c27e28d9fe88da1c86f4b8b1f284765e43c81/third_party/WebKit/Source/platform/bindings/ScriptForbiddenScope.h

You might also consider adding a variant for places that statically know they are on the main thread (like the call site in TreeScope::AdoptIfNeeded).

Also I'm curious what ScriptForbiddenScope in the code base is affecting the performance so much.

Maybe the ScriptForbiddenScope inside DOM mutations?

yukishiino@ said he suspected the CHECK in BeforeCallEnteredCallback. Why isn't it DCHECK?

I suspect it's the scopes in TreeScope and ContainerNode, which happen during each DOM insertion. Before the fix above, the original CL added a TLS access to the constructor and another to the destructor.

The regression is now fixed.

Re: #16

Running a script while it's forbidden may lead to a security issue.  Plus, our test coverage is far from perfect, so we cannot rely on DCHECK, I think.  That's why it's CHECK instead of DCHECK.