1873129 - 7.2% regression on AWFY-Jetstream2-OfflineAssembler-Average around 2Jan2024

Status: RESOLVED INVALID

(Core :: Performance: General, defect, P3)

Description

[Mayank Bansal]

See this :

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3911298,1820228934&series=mozilla-central,3737911,1,13&series=autoland,3911298,1,13&timerange=5184000&zoom=1704192382788,1704434794863,38.0877724592584,81.1510264930627

Regression range : https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6aab4af4cab879cc95debd549578bc909a21cff7&tochange=c5a68fbf81d038a69c4e9be728d50030fd0d6271

Suspects : bug 1867359

I dont think this is a important regression. If the regressing bug is correct, the wins on SP3 far outweigh this minor regression on a subset of Jetstream2.

Comment 1

[Iain Ireland [:iain]]

Comparing profiles with the optimization enabled/disabled, the main difference seems to be a significant increase in the number of samples in TenuringTracer::relocateDependentStringChars -> HeaderWord::isForwarded, from ~40000 (23%) -> ~60000 (30%). So it's not that we're wasting time atomizing eagerly, it's that somehow we've increased GC overhead. I'm not sure how to interpret that.

The hottest function where the optimization kicks in is parseExpressionAtom, which appears to inline a call to isInstruction, which calls Set.has. As far as I can tell, the input strings are all taken from the tokens array, which is populated in lex using strings returned from a regex. So we have an array of objects containing dependent strings, we atomize those strings because we're using them as keys into a Set, and (because of the new optimization) we update the objects to point to the atom instead of the dependent string.

Somehow, this causes problems. I'm not entirely sure how. It is maybe relevant that we're once again looking at tenuring dependent strings; I think that's appeared as a problem before.

On net I think this optimization is still good, but I'll keep this open for now in case we come up with a way to improve things.

Comment 2

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

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 3

[Frank Doty [:fdoty]]

:iain will this be marked as wontfix for 123? You noted in comment 1 that this should be left open - is more investigation needed to understand why a regression was caused despite improvements in other areas?

Comment 4

[Iain Ireland [:iain]]

I circled back on this to upload some profiles for the GC team, but I couldn't reproduce the regression locally. Looking more closely at the perfherder graph: isn't this actually an improvement? Higher is better on this graph, and we seem to have gone from a score of ~51 to a score of ~55.

I have no idea what I was seeing in my earlier profiles. Maybe I just happened to catch a particularly slow run? I remember thinking that I had reproduced the regression locally, but maybe I made the same mistake of thinking lower numbers were better.

Comment 5

[Mayank Bansal]

(In reply to Iain Ireland [:iain] from comment #4)

I circled back on this to upload some profiles for the GC team, but I couldn't reproduce the regression locally. Looking more closely at the perfherder graph: isn't this actually an improvement? Higher is better on this graph, and we seem to have gone from a score of ~51 to a score of ~55.

You are right, this is an improvement. Not sure why i thought this was a regression.

Comment 6

[Iain Ireland [:iain]]

Well, I'm glad we noticed the improvement.