Closed Bug 1444473 Opened 2 years ago Closed 2 years ago

Spectre mitigations for C++ calls from ICs.

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This is a follow-up from Bug 1438886, to support ICs calls too, by adding fences after returning from the call with a potentially speculated value.

(see Bug 1438886 comment 13)
Priority: -- → P1
Megamorphic IC stubs are very perf-sensitive. Can you measure the perf overhead on some micro-benchmarks?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Jan de Mooij [:jandem] from comment #2)
> Megamorphic IC stubs are very perf-sensitive. Can you measure the perf
> overhead on some micro-benchmarks?

Actually I don't know if the megamorphic ICs need this because slot loads do obj->slots[shape->slot()], so the slot number has to come from the shape. Unfortunately there might be confusion with fixed slots vs dynamic slots but because numFixedSlots is stored in the same word I don't know if this is a real risk in practice. Many instructions would have to be executed speculatively to return to IC code, then to the caller, then use the speculative value.

I'm just worried that LFENCE will make megamorphic lookups > 2x slower.
Bug 835306 micro benchmark:

Before:
 ext | num |  Caf  |  Car  |  Cam  |  CAf  |  CAr  |  CAm  |  caf  |  car  |  cam  |  cAf  |  cAr  |  cAm  |
 10  |  1  | 5.392 | 5.220 | 5.756 | 5.135 | 5.144 | 5.370 | 5.002 | 5.056 | 5.302 | 4.899 | 4.940 | 5.520 |
 10  |  2  | 4.946 | 5.028 | 5.612 | 5.177 | 5.883 | 5.978 | 38.54 | 38.43 | 38.42 | 38.15 | 38.65 | 39.02 |
 10  |  4  | 5.205 | 6.175 | 7.228 | 5.429 | 5.648 | 6.788 | 42.20 | 41.75 | 42.43 | 42.05 | 43.06 | 42.86 |
 10  |  6  | 80.38 | 80.74 | 74.36 | 75.20 | 74.94 | 74.40 | 75.67 | 75.48 | 73.20 | 81.65 | 78.60 | 72.77 |
 10  |  8  | 258.0 | 258.3 | 233.0 | 343.1 | 333.8 | 291.6 | 258.6 | 256.3 | 216.5 | 335.0 | 333.1 | 303.0 |
 10  | 12  | 274.7 | 262.1 | 213.6 | 336.7 | 338.8 | 286.3 | 265.6 | 259.3 | 222.3 | 332.1 | 323.3 | 307.1 |
 10  | 16  | 269.3 | 261.1 | 213.1 | 316.5 | 337.6 | 290.7 | 277.0 | 255.2 | 218.6 | 338.5 | 330.4 | 275.6 |
 10  | 24  | 258.8 | 258.7 | 223.1 | 293.1 | 309.7 | 248.6 | 255.8 | 259.1 | 213.3 | 308.2 | 304.9 | 248.4 |
 10  | 32  | 255.5 | 252.4 | 211.1 | 298.3 | 300.4 | 255.4 | 256.5 | 255.2 | 217.2 | 303.5 | 299.2 | 246.3 |

After:
 ext | num |  Caf  |  Car  |  Cam  |  CAf  |  CAr  |  CAm  |  caf  |  car  |  cam  |  cAf  |  cAr  |  cAm  |
 10  |  1  | 5.498 | 5.107 | 5.338 | 5.095 | 5.131 | 7.208 | 5.322 | 5.086 | 5.333 | 5.002 | 5.041 | 5.273 |
 10  |  2  | 5.065 | 4.992 | 5.482 | 5.035 | 5.596 | 7.905 | 38.28 | 38.44 | 37.44 | 37.61 | 37.54 | 37.57 |
 10  |  4  | 4.975 | 6.126 | 8.029 | 6.338 | 5.622 | 6.775 | 42.68 | 43.00 | 43.62 | 42.55 | 42.62 | 43.41 |
 10  |  6  |*83.97 | 81.58 | 75.21 | 75.60 | 74.25 | 73.48 | 75.36 | 76.26 | 72.23 | 81.68 | 78.33 | 70.98 |
 10  |  8  |*304.5 |*295.3 |*244.6 |*377.1 |*364.2 |*327.2 |*303.0 |*299.3 |*248.6 |*381.5 |*366.4 |*470.0 |
 10  | 12  |*304.2 |*299.6 |*245.2 |*383.5 |*363.4 |*319.7 |*300.9 |*298.6 |*249.4 |*371.0 |*362.5 |*330.1 |
 10  | 16  |*300.9 |*296.8 |*250.8 |*356.2 |*371.9 |*308.4 |*305.0 |*297.3 |*249.9 |*365.7 |*363.3 |*304.2 |
 10  | 24  |*305.1 |*293.8 |*244.7 |*349.3 |*348.1 |*279.8 |*313.1 |*295.9 |*247.8 |*351.5 |*353.1 |*279.9 |
 10  | 32  |*307.3 |*306.3 |*249.6 |*347.9 |*343.6 |*278.1 |*306.4 |*320.1 |*249.3 |*341.5 |*348.9 |*277.1 |
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8960545 [details] [diff] [review]
Spectre: Add Jit inline caches mitigation for values returned from C++.

Review of attachment 8960545 [details] [diff] [review]:
-----------------------------------------------------------------

That's a smaller regression than I thought it would be but still >= 10%

We can keep an eye on AWFY and Talos, if there's a significant regression we can reconsider.
Attachment #8960545 - Flags: review?(jdemooij) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/814eccdff53e
Spectre: Add Jit inline caches mitigation for values returned from C++. r=jandem
https://hg.mozilla.org/mozilla-central/rev/814eccdff53e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8960545 [details] [diff] [review]
Spectre: Add Jit inline caches mitigation for values returned from C++.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: We want to uplift Spectre fixes in 61 to 60.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None. (depends on Bug 1431829)
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Add similar speculation barriers as Bug 1431829.
[String changes made/needed]: None.
Attachment #8960545 - Flags: approval-mozilla-beta?
Assignee: nobody → nicolas.b.pierron
Comment on attachment 8960545 [details] [diff] [review]
Spectre: Add Jit inline caches mitigation for values returned from C++.

Spectre mitigation needed for 60. Approved for 60.0b11.
Attachment #8960545 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.