1855661 - 0.15% installer size (OSX) regression on Mon September 25 2023

Status: RESOLVED WONTFIX

(Core :: Gecko Profiler, defect, P2)

Description

[Alex Finder]

Perfherder has detected a build_metrics performance regression from push 446af5901170298be55e94db0c854ec06c6c0102. 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)
0.15%	installer size	osx-cross		87,771,602.54 -> 87,905,168.92

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) may be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

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

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

Comment 2

[Markus Stange [:mstange]]

I've started looking into this but haven't made much progress yet. I confirmed that many of the DOM binding functions (especially getters and setters) have grown by one or two arm64 instructions, because there's one extra register that's spilled to the stack. I also saw a difference in how the ProfilingStack::ensureCapacitySlow() call was handled. I haven't fully understood it yet though.

I was trying to find a very short function that shows the difference, but haven't found one so far. I've only found functions > 300 bytes where the difference shows; I think that makes sense because short functions usually have more available registers and wouldn't need to spill.

In the meantime, I've pushed some try pushes to see whether it was the first or the second patch in the series that caused the difference. I've also made a push which removes the MOZ_UNLIKELY that's added in the second patch.

We could back out bug 1853720 on Beta for now, or we could accept this code size regression for one release. I don't have a strong opinion on which way to go.

Comment 3

[Dianna Smith [:diannaS]]

We are going to accept this for one cycle and try to get a fix in in Fx121

Comment 4

[Ryan VanderMeulen [:RyanVM]]

This doesn't feel like an S2 to me.

(Serious) Major functionality/product severely impaired or a high impact issue and a satisfactory workaround does not exist

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

@mstange any updates on this?

Comment 6

[Markus Stange [:mstange]]

I've picked this up again.

(In reply to Markus Stange [:mstange] from comment #2)

In the meantime, I've pushed some try pushes to see whether it was the first or the second patch in the series that caused the difference. I've also made a push which removes the MOZ_UNLIKELY that's added in the second patch.

Unfortunately I didn't write down the results from these experiments so I'll have to do them again :-/

Comment 7

[Markus Stange [:mstange]]

I forgot about this regression again. At this point I think it's not worth spending more time on the investigation - it always takes forever and the gain from fixing it is quite low.