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

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


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

chromium-rel-win10

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

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

Hi mlippautz@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 : Michael Lippautz
  Commit : c56ae86ad852cd0b7af931e42f7ee29308b79574
  Date   : Wed Aug 02 14:02:56 2017
  Subject: Keep WorkerGlobalScope wrapper alive as long as its context is alive

Bisect Details
  Configuration: winx64_10_perf_bisect
  Benchmark    : system_health.memory_desktop
  Metric       : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_news/load_news_qq
  Change       : 11.95% | 121091413.333 -> 135558485.333

Revision             Result                     N
chromium@491273      121091413 +- 10626806      6      good
chromium@491326      122227371 +- 10838104      6      good
chromium@491352      119169024 +- 6744798       6      good
chromium@491359      119169024 +- 11949857      6      good
chromium@491362      118644736 +- 6905890       6      good
chromium@491363      136344917 +- 10812712      6      bad       <--
chromium@491364      134777523 +- 6091739       6      bad
chromium@491365      135296341 +- 8076985       6      bad
chromium@491378      135558485 +- 3738042       6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

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 --story-filter=load.news.qq system_health.memory_desktop

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

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


For feedback, file a bug with component Speed>Bisection

This was sort of expected, as we keep the scope now longer alive than we did before.

I am inclined to WontFix this one.

nhiroki: Do you think this is going to be a real regression?

This regression is sort of expected but I want to make sure that this won't affect real users much.

I don't know how representative this metric is, +perezju@ (the metric owner) for perspective, WDYT?

The alerts grouped in #0 include 7MiB - 14MiB regressions in v8, which sound pretty significant on metric that is indeed important.

+primiano in case you have more thoughts. Also this is in desktop (windows in particular), not sure who would be the right folks to assess this?

It also has to be noted that (a) lifetime of the scope object was not correct before that CL and (b) a follow up that required this fix already landed.

The regression itself seems to be WAI given the title of the CL and the commentary above.
Would have been nice if the CL had a bug attached or an explanation of the rationale. My reaction to this is: "why are we extending the lifetime of these wrappers and keeping memory around for longer? Is there a performance / energy / whatever benefit that derives from that?" 

+erikchen as this affects desktop benchmarks.

 Issue 752522  has been merged into this issue.

Let me clarify a bit more here.

> The regression itself seems to be WAI given the title of the CL and the commentary above.

Correct, although I would've hoped that it is not noticeable on our benchmarks.

> Would have been nice if the CL had a bug attached or an explanation of the rationale. 

The CL and the linked CL contain quite some context.

> My reaction to this is: "why are we extending the lifetime of these wrappers and keeping memory around for longer? Is there a performance / energy / whatever benefit that derives from that?" 

The lifetime of this scope only extended in the sense that it should've always been as long as the context is alive. The fact that we collected it, and together all its retained wrappers, after no more timers were active meant that we couldn't fire event listeners. The reason this still sort of worked was that there was some duct tape in place where the event listeners were manually put in hidden fields on V8 objects. Keeping objects strongly alive from hidden objects is prone to leaks and essentially doubles the work, that's why we wanted to remove it.

The new lifetime is more correct and less error-prone, so I'd prefer keeping the patch if the detected regression is not a real one. If the regression is a real one, we'll need to rethink about the approach.

REG c#5:

> nhiroki: Do you think this is going to be a real regression?

As I commented on the code review[1], I don't know the number but this could happen on some real world apps that don't explicitly terminate an unused dedicated worker[2]. IMHO, they should explicitly terminate it to avoid such memory consumption.

[1] https://chromium-review.googlesource.com/c/584875#message-5dc2f046e540055610222485dc10a9e45bde074d
[2] This shouldn't happen on shared worker and service worker because they have different lifetime.

Ah I see looks like code health vs memory tradeoff. 
+benhenry for these sort of tradeoffs here.
Given that this has been out there for one week, did anything interesting show up in uma?

Just to add: code health & correctness vs. memory :)

I don't see anything on Dev yet. Unless I'm looking at a different thing: https://uma.googleplex.com/p/chrome/timeline_v2/?sid=7a9afd353d5c1524ac7839ba3624fc46

 Issue 753994  has been merged into this issue.

benhenry: can you take another look?

Looks like it showed up in 3188, is that right? https://uma.googleplex.com/p/chrome/timeline_v2/?sid=b80550e27fd61d8dbd1ab37fa11a4d5f

If so, that's a ~20MB regression at the 50%ile, which seems pretty significant for code health/correctness. haraken@ - do you have any ideas on how to improve the memory usage?

haraken: ping on #19

Accepting this tradeoff, as it is for correctness.