1508550 - StackMapGenerator::createStackMap: don't clone the MachineStackTracker

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Julian Seward [:jseward]]

Currently BaseCompiler::createStackMap creates a temporary clone of
BaseCompiler::mst in the process of making a stack map.  The clone is
stack-local but nevertheless may involve heap allocation.  Better would to not
do the clone, but instead remember the current high-water mark on |mst|, add
entries to that, and then roll back to that point at the end.

Care would have to be taken that we really don't modify entries in the
pre-existing section, and also that the rollback is performed on all error
return paths.

Comment 1

[Lars T Hansen [:lth]]

A halfway point is to just have two statically allocated MSTs, one immutable representing the initial state, which is then copied into the other one everywhere we currently clone.  If we take care not to deallocate the underlying array storage of the latter we should get most of the benefit with not too much of the risk.

(I'm fine with just a single MST too, of course, if you think the complexity is manageable.)

Comment 2

[Lars T Hansen [:lth]]

BTW how is this related to bug 1498470?

Comment 3

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #0)
> Care would have to be taken that we really don't modify entries in the
> pre-existing section, and also that the rollback is performed on all error
> return paths.

Alas, this doesn't work on arm64.  On arm32, x86 and x86_64 it appears to
work OK.  On arm64 the area of the MST that should remain unchanged doesn't
remain unchanged.  I suspect that this is to do with the chunky-stack
arrangment, having the effect that eval-stack items in memory may live at
further up the machine stack than the address implied by masm.frameOffset()
at entry to the body.

I'll look at the plan B -- dual MSTs.

Comment 5

[Julian Seward [:jseward]]

(In reply to Lars T Hansen [:lth] from comment #2)
> BTW how is this related to bug 1498470?

Hmm, it's a dup.  I closed it.

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1508550-dual-MSTs-3.diff

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 9030632 [details] [diff] [review]
bug1508550-dual-MSTs-3.diff

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

Well, that's pretty clean...

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +2001,5 @@
>  #endif
>    }
>  
>    // Clone this MachineStackTracker, writing the result at |dst|.
> +  MOZ_MUST_USE bool cloneTo(MachineStackTracker& dst) {

SpiderMonkey style is actually that arguments that are modified are passed by pointer.  (I don't necessarily agree with that style, I'm just reporting what it is :-)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75aa91b1fca1
StackMapGenerator::createStackMap: don't clone the MachineStackTracker.  r=lth.

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/75aa91b1fca1