1739990 - 74.66 - 68.02% damp custom.jsdebugger.stepIn.DAMP / damp custom.jsdebugger.stepIn.DAMP + 6 more (Linux, OSX, Windows) regression on Tue November 2 2021

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Perfherder has detected a devtools performance regression from push d6033db719c8e92a125fa6daee11533ae1437112. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
75%	damp custom.jsdebugger.stepIn.DAMP	linux1804-64-shippable-qr	e10s stylo webrender	735.47 -> 1,284.58
75%	damp custom.jsdebugger.stepIn.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender	746.49 -> 1,303.22
74%	damp custom.jsdebugger.stepIn.DAMP	windows10-64-shippable-qr	e10s stylo webrender	745.31 -> 1,299.69
72%	damp custom.jsdebugger.stepIn.DAMP	windows10-64-shippable-qr	e10s stylo webrender-sw	747.69 -> 1,288.97
72%	damp custom.jsdebugger.stepIn.DAMP	macosx1015-64-shippable-qr	e10s stylo webrender	870.44 -> 1,495.69
71%	damp custom.jsdebugger.stepIn.DAMP	linux1804-64-shippable-qr	e10s stylo webrender-sw	726.26 -> 1,240.33
71%	damp custom.jsdebugger.stepIn.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender	739.37 -> 1,261.57
68%	damp custom.jsdebugger.stepIn.DAMP	macosx1015-64-shippable-qr	e10s stylo webrender-sw	885.47 -> 1,487.77

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Hi Nicolas, from testing locally this was regressed by Bug 1737993.

It seems the lodash without helper performs better than filter + includes.
without is mostly using this baseDifference helper: https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L2764

And from what I can tell it simply implements an efficient way to exclude items from arrayB in arrayA, which might be difficult to achieve with readable code using filter/includes.

But the regression seems significant enough to warrant a fix here? What do you think?

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO - back January 2nd]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

Hi Nicolas, from testing locally this was regressed by Bug 1737993.

It seems the lodash without helper performs better than filter + includes.
without is mostly using this baseDifference helper: https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L2764

And from what I can tell it simply implements an efficient way to exclude items from arrayB in arrayA, which might be difficult to achieve with readable code using filter/includes.

But the regression seems significant enough to warrant a fix here? What do you think?

Wow, the regression is pretty big!
We should definitely look into this and either:

• extract without (and baseDifference I guess) in a shared file
• see if this is called too often, or if there's another way of doing what we want

I'll look into it

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1737993

Comment 4

[Nicolas Chevobbe [:nchevobbe][PTO - back January 2nd]]

Attachment: Bug 1739990 - Fix perf regression in getInScopeLines. r=jdescottes.

The patch for Bug 1737993 did a very readable but not performant replacement
of lodash without usage in getInScopeLines.
This patch changes things a bit so we get a similar level of performance as
what we had before, when using without.
For this we :

• make getOutOfScopeLines returns a Set so checking inclusion is faster
• create the final array of "in scope lines" using a for loop and assigning
  line number when needed, and then filtering out empty slots.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4789d873d1cb
Fix perf regression in getInScopeLines. r=jdescottes.

Comment 6

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/4789d873d1cb