1771223 - Enable "-ftrivial-auto-var-init=pattern" for code in dom component

Status: NEW

(Firefox Build System :: Toolchains, enhancement)

Description

[Tyson Smith [:tsmith]]

Enable "-ftrivial-auto-var-init=pattern" on individual components and sub-components. This will help contain any performance regressions to a known area making it easier to identify, locate and address them. It will be easier to coordinate to address issues that are found without blocking the use of the feature with components. The same logic applies to problematic areas/components that introduce an unreasonable overhead that is determined to be unfixable.

Prefherder results are available here:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=6bef577992bd17a096a6ff5d6cabda38b711998e&newProject=try&newRevision=3ecc3879419e27f46c22e9b7072c8c0068b235e4&framework=13&page=1&pageTitle=%5Bdom%5D+-ftrivial-auto-var-init%3Dpattern&showOnlyImportant=1

Comment 1

[Alex Thayer [:alexical] (she/her)]

I worry about the possibility of this allowing many small performance regressions through the cracks, resulting in one large performance regression overall, just spread out over time. Are we taking any extra care to mitigate this effect?

Comment 2

[Tyson Smith [:tsmith]]

(In reply to Doug Thayer [:dthayer] (he/him) from comment #1)

I worry about the possibility of this allowing many small performance regressions through the cracks, resulting in one large performance regression overall just spread out over time.

Due to the nature of this mitigation there may be minor regressions. Applying to the entire code base at once has already been shown to be unrealistic. By adding the ability to enable the mitigation on a per module basis (Bug 1769118) it gives us (module owners, security team, performance team) the ability to evaluate the cost/benefit on a per component/sub-component basis. This will help contain any performance regressions to a known area making it easier to identify, locate and address them. It will be easier to coordinate with teams to address issues that are found without blocking the use of the feature on other components. The same logic applies to problematic areas/components that introduce an unreasonable overhead that are determined to be unfixable. This should hopefully allow us to ship the feature bit by
bit and get the benefits as we go.

This is a rather large body of code and I would expect that the performance tests would highlight any concerns, is this not true?

Are we taking any extra care to mitigate this effect?

What did you have in mind?

Comment 3

[Tyson Smith [:tsmith]]

I would like to move forward with enabling this feature to give it a much soak time in m-c as possible. Looking at the Perfherder results it looks there are only a couple minor performance regressions (<2% or <10ms). This seems acceptable considering the security benefit but please correct me if I am wrong.

:hsinyi, is there anyone else that should notified before continuing?

Bug 1769128 enabled the feature for editor last week.

Comment 4

[Bobby Holley (:bholley)]

Generally speaking I think it makes more sense to partner with the perf team on this initiative rather than specific component owners. Switching NI from Hsin-Yi to Patricia to reflect that.

My intuition is that the performance impact of this mitigation is likely to be concentrated in a handful of hot currently-uninitialized variables rather than spread diffusely across all of them. While we might consider accepting some tiny performance regression for the security benefits, it would be a missed opportunity not to try to identify these hot instances.

A mechanical way to do this is through bisection. First, we enumerate the regressions we see with the top-level change. Next, we apply the change only to half the code, and recursively investigate the subset that appears to have disproportionate impact. If the regression is in fact diffuse this won't work, but if it's concentrated in a few places this technique should at least lead us to the relevant file.

Comment 5

[Bobby Holley (:bholley)]

Another way to assess the performance impact would be to instrument the generated variable initialization to include an out-of-line call to a singleton function that does a tunable amount of busywork. We'd then profile a workload, filter for that function, and look for the hottest callers.

Whether clang gives us the hooks to instrument things in this way, I'm not sure.

Comment 6

[Tyson Smith [:tsmith]]

(In reply to Bobby Holley (:bholley) from comment #4)

While we might consider accepting some tiny performance regression for the security benefits, it would be a missed opportunity not to try to identify these hot instances.

Are you asking to hold off on enabling on DOM?

Is there something specific in the results that you are referring to?

If there is nothing specific, could the investigation be scheduled in a follow up bug (assuming we don't see any unacceptable performance regressions)?

I do agree with the points and suggestions I'm just unclear on the level of concern :)

Comment 7

[Bobby Holley (:bholley)]

I think there is value in trying to understand the performance impact before the change lands rather than after-the-fact. So I'd like us to do some amount of bounded investigation of this problem before enabling it for more modules. Given that Doug figured out how to patch the relevant parts of clang in bug 1771996, the approach in comment 5 might be reasonably tractable.

The tp5o_responsiveness regression seems real and self-contained enough to be worth investigating with bisection, though I also see a similar regression in the try push in bug 1769128 so worth figuring out what the deal there is.

Comment 8

[Bobby Holley (:bholley)]

And of course since the bisection thing is targeted at specific regressions, it will scale less well to other components than generalized instrumentation. I think we should still try it out, but that the latter is probably more valuable if we can get it hooked up.

Comment 9

[Tom Ritter [:tjr]]

I think what we'd be trying to do is change the codegen in these places; it might be something like CGF.Builder.CreateCall(), but from here I'd need to do a lot more experimenting on how to emit a call to a defined function...

Comment 10

[Bobby Holley (:bholley)]

I forked out bug 1772353 to investigate the approach from comment 5.

Comment 11

[Patricia Lawless]

removing old ni (I see Alex in the comments of the investigation bug, but LMK if there is anything still needed here)