1583172 - Define/set BaselineFrame::frameSize_ only in DEBUG builds

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

When calling into the VM, BaselineFrame::frameSize_ is very similar to the frame size stored in the frame descriptor. The main difference is that frameSize_ does not include the size of VM function arguments when calling a VM wrapper so it's used for determining the number of Values on the BaselineFrame's local/expression stack. It's also more easily available given just a BaselineFrame* pointer.

The duplication has been bothering me for a while because it involves a store and (in some cases) some calculation for each VM call from Baseline code.

It's possible to make BaselineFrame::frameSize_ DEBUG-only and rename it (nice for assertions for now), make JSJitFrameIter compute the equivalent frame size, and pass the frame size explicitly to some VM calls where we don't want frame iteration. I have some prototype patches for this and it works nicely.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 1 - Remove always-nullptr arguments from jit::Recompile. r?tcampbell!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 2 - Add baselineFrameNumValueSlots method to JSJitFrameIter and use it in a few places. r?tcampbell!

This just forwards to BaselineFrame for now but that will change.

Depends on D46911

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 3 - Make JSJitFrameIter compute the Baseline frame size. r?tcampbell!

The computed value is equivalent to BaselineFrame::frameSize_, but later in the stack
the BaselineFrame::frameSize_ field will be debug-only.

Depends on D46912

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 4 - Pass frame size explicitly to jit::NormalSuspend. r?tcampbell!

For now we just assert it matches the size stored in the frame.

Depends on D46913

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 5 - Store frame size in BaselineBailoutInfo. r?tcampbell!

Depends on D46914

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 6 - Pass frame size to Ion's NewBaselineFrameInspector. r?tcampbell!

One of the more tricky parts: Ion's NewBaselineFrameInspector needs to know the
frmae size for numValueSlots so this patch passes down a BaselineFrameWithSize
struct that stores both the frame and the size.

Depends on D46916

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1583172 part 7 - Rename BaselineFrame's frameSize_ field to debugFrameSize_ and define/set it only in debug builds. r?tcampbell!

Depends on D46917

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92dca2ee7dd3
part 1 - Remove always-nullptr arguments from jit::Recompile. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/e94031916ebf
part 2 - Add baselineFrameNumValueSlots method to JSJitFrameIter and use it in a few places. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/71e47d3ce0f3
part 3 - Make JSJitFrameIter compute the Baseline frame size. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/d337e9229327
part 4 - Pass frame size explicitly to jit::NormalSuspend. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/de54dcbfcf74
part 5 - Store frame size in BaselineBailoutInfo. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/690f3798bbaf
part 6 - Pass frame size to Ion's NewBaselineFrameInspector. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/4f2cba0b03be
part 7 - Rename BaselineFrame's frameSize_ field to debugFrameSize_ and define/set it only in debug builds. r=tcampbell

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out 7 changesets (Bug 1583172) for build bustages at SharedICHelpers-x86-inl.h.

Backout: https://hg.mozilla.org/integration/autoland/rev/3954181d653874b513b531d8a9704612080e58e3

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=4f2cba0b03be7de53dda9534738cb872259dbf23&selectedJob=269333722

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269333722&repo=autoland&lineNumber=26522

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0195e903299
part 1 - Remove always-nullptr arguments from jit::Recompile. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8a111dcc2a98
part 2 - Add baselineFrameNumValueSlots method to JSJitFrameIter and use it in a few places. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/c75007e8327a
part 3 - Make JSJitFrameIter compute the Baseline frame size. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/6b5de6b58a89
part 4 - Pass frame size explicitly to jit::NormalSuspend. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/50e944d9d99c
part 5 - Store frame size in BaselineBailoutInfo. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/5937ca265a0b
part 6 - Pass frame size to Ion's NewBaselineFrameInspector. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/b2feeb41df2b
part 7 - Rename BaselineFrame's frameSize_ field to debugFrameSize_ and define/set it only in debug builds. r=tcampbell

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e0195e903299
https://hg.mozilla.org/mozilla-central/rev/8a111dcc2a98
https://hg.mozilla.org/mozilla-central/rev/c75007e8327a
https://hg.mozilla.org/mozilla-central/rev/6b5de6b58a89
https://hg.mozilla.org/mozilla-central/rev/50e944d9d99c
https://hg.mozilla.org/mozilla-central/rev/5937ca265a0b
https://hg.mozilla.org/mozilla-central/rev/b2feeb41df2b