1386398 - stylo: 8.68 - 13% Strings Stylo Servo_DeclarationBlock_SetPropertyById_Bench / Stylo Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench (osx-10-10) regression on push 2c6f632e13263b78cae4e6822acf4e48e6a054c2 (Mon Jul 31 2017)

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect, P2)

Description

[William Lachance (:wlach)]

Hey Josh, I noticed this while validating some work I did earlier on tightening up the platform microbench tests. Looking at the graphs, the regressions seem real (although the regression seems strangely localized to Mac). Bug 1381143 is the only likely culprit.

These "microbenchmark" alerts have no official backout policy, please handle as you see fit.

--

We have detected a platform microbenchmarks regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=2c6f632e13263b78cae4e6822acf4e48e6a054c2

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 13%  Stylo Servo_DeclarationBlock_SetPropertyById_Bench osx-10-10 opt      469,793.67 -> 528,575.83
  9%  Stylo Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench osx-10-10 opt 655,128.75 -> 711,982.17


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=8457

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Platform_Microbenchmarks

Comment 1

[Josh Matthews [:jdm]]

Fun fact - the benchmark code should not be executing any error handling code, yet demonstrates a slowdown from a patch which only modifies error handling code. The slowdown is entirely caused by adding a field to PropertyDeclarationError::InvalidValue; even just a u32 demonstrates the same performance regression.

Comment 2

[Bobby Holley (:bholley)]

That gives a lot of credibility to Simon's theory that the large return types are hurting us in parsing!

Comment 3

[Ionuț Goldan [:igoldan]]

Guys, how are we doing on this bug?

Comment 4

[Josh Matthews [:jdm]]

Investigations are ongoing. There is no specific fix for this benchmark regression at this time.

Comment 5

[Josh Matthews [:jdm]]

As a confusing counterpoint to comment 1 and comment 2, I have some patches which make all of the parsing code return results containing BasicParseError or () as the error type. The gecko profiler results for loading the myspace page demonstrate identical timings in the majority of cases. I have no idea why I'm not seeing any performance improvement with that change.

Comment 6

[Bobby Holley (:bholley)]

Hmm. Can you still reproduce your results from comment 1, using whatever methodology and revision you used at the time?

Comment 7

[Bobby Holley (:bholley)]

<jdm>	so my opinion is that while I was able to reproduce some kind of smaller benchmark numbers when changing the return value size, I did not actually see a corresponding change in real workloads
<jdm>	therefore, I don't think that alert is useful to keep pursuing


It sounds like this report probably isn't useful anymore. Bug 1381270 comment 10 indicates that error reporter handling is looking better now. We're still working on parser performance, but we can probably resolve this.