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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7pKcsAsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7rWisgsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7u-voQoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7v3dvAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7q2oqQsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7oPqugoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7qXDqAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7v7c6AsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgnvyL-gkM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgnpDzmggM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgnsiorAoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg7uH-rwkM


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

chromium-rel-mac-retina
chromium-rel-mac11-pro
chromium-rel-mac12
chromium-rel-win7-dual
chromium-rel-win7-x64-dual

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

=== Auto-CCing suspected CL author rune@opera.com ===

Hi rune@opera.com, 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 : Rune Lillesveen
  Commit : 334b4846828ceb697785b466619a1f1b96592a3b
  Date   : Wed Jun 28 12:08:58 2017
  Subject: Separate initial style and viewport/ICB style.

Bisect Details
  Configuration: mac_retina_perf_bisect
  Benchmark    : loading.desktop
  Metric       : cpuTimeToFirstMeaningfulPaint_avg/pcv1-cold/FarsNews
  Change       : 373.23% | 573.416333333 -> 2713.59966667

Revision             Result                  N
chromium@482907      573.416 +- 52.1976      6      good
chromium@482933      568.094 +- 28.0935      6      good
chromium@482946      565.463 +- 24.5767      6      good
chromium@482948      568.639 +- 28.1729      6      good
chromium@482949      565.387 +- 16.6433      6      good
chromium@482950      2694.37 +- 104.143      6      bad       <--
chromium@482953      2701.93 +- 78.2153      6      bad
chromium@482959      2713.6 +- 70.8511       6      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 --story-filter=FarsNews loading.desktop

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

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


For feedback, file a bug with component Speed>Bisection

Explictly assigning. A CL you landed tripped one of the speed metrics we measure in the lab. If this is the first time this has happened to one of your CLs, or if it's been a while, please read: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md

We're looking for one of the following:
1. Justification via explanation
2. Plan to revert or fix
3. Angry rage throwing of equipment at my head

Just be aware that I'm trained in trumpet playing and First Aid and am not afraid to use it.

Note: This was a bulk edit message and not very personal.

I do have a hope that this will fix it: https://chromium-review.googlesource.com/c/606007

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

commit c3d15c145666e6b393ffdbdbdb9727285e6aecae
Author: Rune Lillesveen <rune@opera.com>
Date: Wed Aug 09 08:07:08 2017

No need for recalc on viewport propagation.

Before the fix for  issue 732349 , we relied on propagation from body to
viewport to affect the computed style of the root element which was
handled through a subtree recalc after viewport propagation. This is no
longer necessary. In fact, when rtl was specified on body, but the
computed style of html was ltr, InheritHtmlAndBodyElementStyles would
always trigger a subtree recalc, which would happen every frame we had a
style recalc.

There's a hope this will fix performance  issue 739133 .

I think that InheritHtmlAndBodyElementStyles can be made into a
PropagateStyleToViewport method which can be called at the end of
style recalc to avoid calculating html and body style twice. I'll try
to do that in a separate CL.

Bug:  739133 
Change-Id: I0beebcf850661434eedb8bd19405698c27b7ae89
Reviewed-on: https://chromium-review.googlesource.com/606007
Commit-Queue: Rune Lillesveen <rune@opera.com>
Reviewed-by: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/heads/master@{#492889}
[modify] https://crrev.com/c3d15c145666e6b393ffdbdbdb9727285e6aecae/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/c3d15c145666e6b393ffdbdbdb9727285e6aecae/third_party/WebKit/Source/core/dom/DocumentTest.cpp

Pls apply appropriate OSs label.

Also before we approve merge to M61, could you pls confirm change listed at #6 will be a safe merge to M61? Pls note M61 is in Stable so merge bar is extremely high.

It looks like it should affect all the OS. The perf regression was recorded for Mac and Widows, but  bug 762132  was reported for Linux as well. 

And, perf bot that caught this regression appears to run only on desktop. (we should have something similar for Android). 
 
+eae, kojii : What do you think of merging to branch? It should fix a very serious performance loss for RTL web pages (e.g. Youtube, Facebook in Hebrew,Arabic,Farsi)

For my untrained eyes, it looks safe, but it does not carry much weight. 

rune@:  Do you have a follow up CL to do what you mentioned in your CL description? 

> I think that InheritHtmlAndBodyElementStyles can be made into a
PropagateStyleToViewport method which can be called at the end of
style recalc to avoid calculating html and body style twice. I'll try
to do that in a separate CL.

+meade (who reviewed a CL that broke it, but meade@ is in UTC+10...) 
+mstensho@opera.com : who reviewed both the original CL and a fix CL.  

I wonder why the two reviewers of the original CL that broke it were NOT added to this regression bug automatically. 

> It looks like it should affect all the OS ...

Yes, this should affect all OSs.

> Also before we approve merge to M61, could you pls confirm change listed at #6 will be a safe merge to M61?

The change is removal of code incorrectly marking root/body for recalc and is fairly contained. I would say pretty safe.

> rune@:  Do you have a follow up CL to do what you mentioned in your CL description?

That is https://crrev.com/9f25abddd6a5e0d25d5082ec0f29727a984f9e03

Should not be relevant for fixing the perf issue, though.

+trchen (who took a look at rune's follow-up CL.). 

Can you give us your take on merging the fix CL ( https://chromium-review.googlesource.com/606007  ) to M61 branch?  Thanks


----------------------

rune@ had a follow-up CL landed the day after the fix CL. 

https://chromium-review.googlesource.com/c/chromium/src/+/608028 is a follow up CL.  Unfortunately, that made it harder to "infer" the safety of the fix CL (https://chromium-review.googlesource.com/606007 ) from the fact that it's been in M62 and M63 branches. 

oh.. thanks, rune@ !  

rune@: do you know any idea why the perf graph at https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB  
does not show any noticeable improvement after your fix CL is landed? 

The average is still much worse than your original CL was landed, but it has a huge  error bar. So, I'm not sure what to make of the graph. 

+benhenry@ & sullivan@ of Perf/Speed team.

The url in comment 19 was cut short. This is the correct URL. 

https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgnpDzmggM  

Did you try extending the bar on the bottom green chart out? I see the improvement (screenshot attached)

Deleted: improvement-showing.png 353 KB

	improvement-showing.png 353 KB View Download

Ooops. Thank you. I misused the tool. 

rune@:
Per comment #15, We only need merge for cl listed at #6 (  https://chromium.googlesource.com/chromium/src.git/+/c3d15c145666e6b393ffdbdbdb9727285e6aecae (62.0.3181.0)) and not follow up CL, correct?

In the above graph, the last bad point was crrev.com/492547 and the first good point is crrev.com/492916, which is BEFORE rune's follow-up CL (which is 493xxx ). 
So, we can confirm that his fix CL alone (without the follow-up CL)  took care of the regression. 

And, rune's fix ( crrev.com/492889) was between the last bad and the first good (as expected). 

mstensho@ is on vacation for 2 weeks, and meade@ is OOO until Oct 3.

The CL looks safe to me, at worst we break layout of RTL or vertical flow documents in some cases our tests don't cover if any. I think it's less severe even if so than this issue.

rune@ is the best expert in this area, I'm afraid I'm not really an expert on this.

+naina@, both naina@ and eae@ are in Tokyo (UTC+9) this week for BlinkOn. If you could wait for ~6 hours I hope she can look.

If this is too urgent to wait for 6 hours, my best comment is as above.

Thank you kojii@. 
I'm ok to take merge in for cl listed at #6 (laded on August 9th) per comments #15, #25 and #27. But want to get +amineer@ input here (Alex, this is needed for  Bug 762132  - Youtube and Facebook are very slow when the site UI language is Hebrew or RTL languages)

Concur with govind@'s assessment, I think the impact of the regression is severe enough to warrant the risk based on the data above and kojii@'s review in c#27.

Thank you amineer@
Approving merge to M61 branch 3163. Please merge ASAP.

I'm starting merge. 

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

commit 3f83de090df47bf2bb3dc8f86eca45f4e5f4a3f6
Author: Rune Lillesveen <rune@opera.com>
Date: Wed Sep 20 20:10:41 2017

[M61 Merge] No need for recalc on viewport propagation.

Before the fix for  issue 732349 , we relied on propagation from body to
viewport to affect the computed style of the root element which was
handled through a subtree recalc after viewport propagation. This is no
longer necessary. In fact, when rtl was specified on body, but the
computed style of html was ltr, InheritHtmlAndBodyElementStyles would
always trigger a subtree recalc, which would happen every frame we had a
style recalc.

There's a hope this will fix performance  issue 739133 .

I think that InheritHtmlAndBodyElementStyles can be made into a
PropagateStyleToViewport method which can be called at the end of
style recalc to avoid calculating html and body style twice. I'll try
to do that in a separate CL.

Bug:  739133 ,  762132 
Change-Id: I0beebcf850661434eedb8bd19405698c27b7ae89
Reviewed-on: https://chromium-review.googlesource.com/606007
Commit-Queue: Rune Lillesveen <rune@opera.com>
Reviewed-by: Morten Stenshorne <mstensho@opera.com>
Cr-Commit-Position: refs/heads/master@{#492889}(cherry picked from commit c3d15c145666e6b393ffdbdbdb9727285e6aecae)

TBR=rune@opera.com

Change-Id: I0beebcf850661434eedb8bd19405698c27b7ae89
Reviewed-on: https://chromium-review.googlesource.com/675594
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1248}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/3f83de090df47bf2bb3dc8f86eca45f4e5f4a3f6/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/3f83de090df47bf2bb3dc8f86eca45f4e5f4a3f6/third_party/WebKit/Source/core/dom/DocumentTest.cpp