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

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


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

chromium-rel-mac11-air

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

=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: mac_air_perf_bisect
  Benchmark    : blink_perf.parser
  Metric       : iframe-append-remove/iframe-append-remove

Revision             Result                  N
chromium@494028      154.321 +- 15.7734      20      good
chromium@494332      153.711 +- 24.9249      21      bad

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

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

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


For feedback, file a bug with component Speed>Bisection

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14e5c739780000

Lots of jumps on this graph. Trying a pinpoint bisect over a wide range. If there's not really multiple regressions, maybe something's wrong with the metric here?

📍 Found significant differences after each of 2 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/14e5c739780000

cocoa: draw default favicon in theme color
By ellyjones@chromium.org · Wed Aug 09 18:53:09 2017
chromium @ e78297f19a802b8e4f26b170f2e4c026a226fba7

Update Chrome Upstream flow (Chrome -> Payments upload credit card save for Autofill) to reflect new UI mocks
By jsaul@google.com · Wed Aug 09 18:54:49 2017
chromium @ 19ef81cf629066734a0fd1d1699d43ee307af659

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Cc-ing blink_perf.parser owner jbroman. Jeremy, take a look at the pinpoint results from #6: https://pinpoint-dot-chromeperf.appspot.com/job/14e5c739780000

Those CLs don't look like they should be related to parser performance.

It seems like the drops we're seeing on the perf dashboard aren't bisecting very well. Any ideas what's wrong with the test?

Lucas, do you have the bandwidth to take a look at this?

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14f6d88d780000

📍 Found significant differences after each of 4 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/14f6d88d780000

Tweak resolution changed notification texts.
By tetsui@chromium.org · Mon Sep 11 01:55:06 2017
chromium @ b798ffec0873acb2427260314138d1cf896eae46

Revert "Catch PlatformSensor::StartSensor failure in Windows tests"
By benwells@chromium.org · Mon Sep 11 02:11:28 2017
chromium @ b53dff9202400a231108ed4e7dd550cbfa229b06

FieldTrial: Add field trial configuration for V8ContextSnapshot
By peria@chromium.org · Mon Sep 11 02:31:08 2017
chromium @ 3e5db563a65e3968d1e336312ae1bbb85e18321d

clear submitted_forms_ in same page navigation
By michaelbai@chromium.org · Wed Sep 13 19:53:51 2017
chromium @ 2b7a1bec8e66b94917d7a18374b16e6424a51120

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

The pinpoint output is noisy because tests failed, but this time it really shows peria's change "FieldTrial: Add field trial configuration for V8ContextSnapshot" as the culprit. Reassigning to lfg per #8.

So, here are my findings:

- The last bisect job was to figure out why the performance has increased back to similar levels as before, and to figure out whether this was a regression that was eventually fixed. This isn't the case, peria@'s change should improve load times, but it's unrelated to this regression.

- The initial bisect is actually correct. The change in e78297f19a802b8e4f26b170f2e4c026a226fba7 causes the browser process to do a significant amount of extra work, which ends up affecting the renderer process. This only shows up in low core count computers, so mac pros are unaffected. See the attached screenshot for before (top)/after (bottom) CPU usage in the browser process.

My limited Mac/objective-C knowledge makes it hard to fix this myself, ellyjones@, would it be possible to set the icon only if the new image is different than the current one in TabController?

Deleted: Screen Shot 2017-10-16 at 5.48.40 PM.png 566 KB

	Screen Shot 2017-10-16 at 5.48.40 PM.png 566 KB View Download

Also, here's another profiler capture of today's canary (64.0.3241.0), that shows that this is still an issue today.

Deleted: Screen Shot 2017-10-16 at 6.11.46 PM.png 305 KB

	Screen Shot 2017-10-16 at 6.11.46 PM.png 305 KB View Download

ellyjones: ping on this, can you take a look at the extra work caused by your CL per #12?

 Issue 756062  has been merged into this issue.

ellyjones: can you take a look at this?

I will look at this this week.

The NextAction date has arrived: 2018-01-19

I'm digging into this today.

I don't immediately understand why my change caused more work, since as far as I can see there's no extra logic to discard redundant image updates, so the previous code that would reuse the default favicon image should have been causing just as many updates. I will investigate further.

Further inspection:

We aren't causing any more calls to [TabController setIconImage:] than there were before; the profiling samples above show that we're spending a ton of time in [SpriteView setImage:], and more specifically in ScopedCAActionDisabler. I think the root cause is that using a ScopedCAActionDisabler here is very slow, but I'm not sure how to progress from there.

ccameron@, you were one of the original reviewers on this code in SpriteView and also are a graphics wizard - do you know what's going on here?

Some background:

CoreAnimation animates changes by default (like, fade-in new content, animates position changes, etc). ScopedCAActionDisabler exists to disable this -- it just specifies a CATransation that says "don't do that, and batch up all of the changes to be committed when the CAActionDisabler goes out of scope.

So, it's whatever we're doing in that block that is the expensive stuff (it just gets charged to the CAActionDisabler dtor).

I suspect that if we remove the CAActionDisabler we will see still the issue -- is that the case?

I'd guess that the cost is -[CALayer setContents:] at
inhttps://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/sprite_view.mm?rcl=ac97cd080598d6c960ac42322e1ef24a05094b10&l=101

What are the properties of the image?
- is it very large and being scaled?
- is there some color space conversion being applied?

I did a little more digging:

1) The largest images that are used are IDR_THROBBER_WAITING (2864x16 at 1x, 5728x32 at 2x) and IDR_THROBBER (576x16 at 1x). We then do an animation that animates the contentRect of the layer across the different images to produce the animation. So, it's not being scaled precisely.

2) The underlying NSBitmapImageReps seem to have ColorSpace=(not yet loaded), so I'm not sure how to tell if a conversion is happening. (I don't know what it means for the colorspace to be "not yet loaded").

With the CAActionDisabler there, it takes O(100k) nanoseconds to run this function when loading the largest image mentioned above. Without the CAActionDisabler, it takes about the same amount of time. Therefore, the CAActionDisabler isn't at fault here.

ccameron: any ideas here? The image seems big but not excessively so - I'm wondering if it could be a lot slower on a Mac Mini or some older piece of hardware, since I was testing on my Mac Pro above.

For completeness, I was getting timestamps using mach_absolute_time(), then converting the time delta to nanoseconds using AbsoluteToNanoseconds().

Couple of thoughts:
1. Is this successfully re-using the same NSImage every frame, or is it re-creating the NSImage at [1] every frame?

2. If indeed we're successfully re-using the NSImage, then it might be reasonable to convert the ui::Image to an IOSurface, since that guarantees that no extra work will be done (lemme know if we get here and I can add some plumbing).

[1] https://cs.chromium.org/chromium/src/ui/gfx/image/image.cc?rcl=05192e8f55c9b4951d360422d1d034a64422efc5&l=480

It looks like shrike@'s <https://chromium-review.googlesource.com/c/chromium/src/+/580131> will actually delete all this code, thus fixing this problem once and for all :)

This is either obsolete or will be made obsolete by views.