1508665 - Rabaldr: clean up BaseStackFrame

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Lars T Hansen [:lth]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1507785#c11 et seq.  BaseStackFrame should be reorganized (either keeping it as one class or splitting it in a base class and a derived class) so that we more clearly can separate the physical stack size ("framePushed") from the logical stack size ("stack height").

The variable called currentFramePushed_ probably should not be called that, because it's related to the stack height.

The method called currentFramePushed() probably should not be called that, for the same reason.  Though note there is already a public method called stackHeight(), so be careful.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1508665-split-basestackframe.patch

This is a large diff because it moves code around quite a bit and renames and clarifies; the actual changes are not all that great however.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9027544 [details] [diff] [review]
bug1508665-split-basestackframe.patch

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

Sorry, I'm sick of doing reviews of changesets which mix up code motion, renamings and edits. Please split it or ask somebody else for review.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 9027544 [details] [diff] [review]
> bug1508665-split-basestackframe.patch
> 
> Review of attachment 9027544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I'm sick of doing reviews of changesets which mix up code motion,
> renamings and edits. Please split it or ask somebody else for review.

I'm sure we're all sick of doing reviews, but that doesn't make the patches go away, does it?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #3)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> > Comment on attachment 9027544 [details] [diff] [review]
> > bug1508665-split-basestackframe.patch
> > 
> > Review of attachment 9027544 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry, I'm sick of doing reviews of changesets which mix up code motion,
> > renamings and edits. Please split it or ask somebody else for review.
> 
> I'm sure we're all sick of doing reviews, but that doesn't make the patches
> go away, does it?

Please correctly read my comment: I'm not sick of doing reviews, I'm sick of doing reviews **WHICH MIX UP CODE MOTION, RENAMINGS AND EDITS**. These make issues very hard to spot and regressions very easy to slip in, increase the patches' size for no good reason, and tend to degrade the overall quality of the code by having reviewers giving up. We've had a long time culture of doing this in WebAssembly code, and I'm not pointing fingers at others, I've been doing this myself too. Nevertheless I think it should stop (time has proven at many times that it was a bad practice).

I'm actually not sick of doing reviews, since I'm hereby proposing to do reviews for three patches: one that does the code motion, one that does the renaming, one that applies functional changes. Conflating these changes into a single patch makes it much more likely that a reviewer would miss things (and that would be the case for any reviewer), so it would be equivalent to a rubber stamp. If you want to get a proper review, please make proper patches.

Comment 5

[Luke Wagner [:luke]]

I agree the proper approach to patches is to separate code motion/renamings from any more-meaningful changes.  I try to do that in general, but I know I'm also the worst offender in the past for doing Mega-Refactoring Patches.

I think there's actually a middle tier here:
 1. pure code motion / renaming, mechanical stuff
 2. refactorings that shouldn't change observable behavior, but aren't so trivial that a bug couldn't slip in
 3. non-refactoring patches where bugs are actually likely
and, in our defense, I think we're all generally pretty good about keeping 3 in separate patches from 1+2 patches, but I completely agree that 1+2 patches are way worse than separate 1 and 2 patches because it's hard to tell, when reading a patch from top to bottom: are you reading mostly-moved code or actual refactored code that requires a look.  And often, it's mostly-moved *but with 1 interesting change hiding in the middle* that you only can see by juxtaposing the src and dst hunks and doing a manual visual diff.  Very annoying.

I think we all strive for separate 1/2/3 patches but it's very easy to start with a 2 patch as your initial goal and in the righteous zeal of "making things better" accidentally do a bit of 1 and by the time you realize you've actually created a non-trivial 1+2 patch, it's often too late to electrolyze separate 1/2 patches without simply redoing the whole thing from scratch.  I expect that's the case here too.

To help dial this situation down a bit and pay back my own 1+2 review debt, I'll review this patch as-is but agree we should all strive to have separate 1/2/3 patches going forward.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

I apologize for the tone of my previous comments, which are more about me venting rather than something personal against anybody else. I had in mind that as a team, we share the goal of making reviews more effective (less cognitive load for the reviewers, faster to get for the reviewees). That being said, we've never explicitly stated this goal as a team, nor how to apply it, so it was inopportune for me to expect this from others in general. Maybe we can discuss this face to face during the All Hands next week, and/or as part of the Spidermonkey discussions.

Luke, thanks for take the review for yourself; I'll take it back, since I was the one who asked for these changes in the first place.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9027544 [details] [diff] [review]
bug1508665-split-basestackframe.patch

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

Looks good, thanks. Drawing the different sections in ASCII art would be really nice to have a very fast intuition of how localSize(), fixedSize(), InitialChunk, currentStackHeight() and masm.framePushed() relate to each other. (I might do it myself, because it tremendously helped to draw it on paper here.)

I think I liked even better the idea of renaming the masm.framePushed() related things to "physical stack" and height related ones to "logical stack", for what it's worth, but this works well enough.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1006,2 @@
>  //  - the Arguments area, comprising memory allocated for outgoing calls,
>  //    allocated below the stack area.

nit: there's a reference to the previously called stack area here

@@ +1221,5 @@
> +
> +    // Record the current stack height, after it has become stable in
> +    // beginFunction().  See also BaseStackFrame::onFixedStackAllocated().
> +
> +    void onFixedStackAllocated() {

Should this function just be merged with setLocalSize? If I understand correctly the local size can't change between the setLocalSize() and the onFixedStackAllocated() calls.

@@ +1248,5 @@
> +    //
> +    // The Dynamic area - the dynamic part of the frame, for spilling and saving
> +    // intermediate values.
> +
> +    // Offset off of sp_ for the slot at stack area location `offset`

nit: dot at the end of sentence

@@ +1256,5 @@
> +    }
> +
> +#ifdef RABALDR_CHUNKY_STACK
> +
> +# define CHUNKY_INVARIANT()                                          \

Could it just be an inline function instead? (no need to make it explicitly inlined as it'd be defined in the class definition)

@@ +1264,5 @@
> +
> +    void pushChunkyBytes(uint32_t bytes) {
> +        CHUNKY_INVARIANT();
> +        if (masm.framePushed() - currentStackHeight_ < bytes) {
> +            masm.reserveStack(ChunkSize);

Just noticed: could it fail if bytes > (masm.framePushed() - currentStackHeight_ + ChunkSize)? I guess it would trigger the chunk invariant asserts below, so it would happen that none of the pushes in the baseline compiler are big. If I'm not misunderstood, might be nice to either generalize this code so it handles bigger bytes size, or add a comment that it works because we only do small pushes? (or MOZ_ASSERT bytes <= ChunkSize as a stricter assertion)

@@ +3575,4 @@
>  
>          fr.checkStack(ABINonArgReg0, BytecodeOffset(func_.lineOrBytecode));
>          masm.reserveStack(fr.fixedSize() - masm.framePushed());
> +        fr.onFixedStackAllocated();

If you agree with the other comment suggesting to merge StackFrameAllocator::onFixedStackAllocated() with setLocalSize, then this can just contain the maxFramePushed_ = masm.framePushed() statement that's remaining in onFixedStackAllocated().

Otherwise:
This name is a bit inconsistent with the rest of other names. Maybe finishFixedStack? settleFixedStack? setFixedStack? or something else, could be more in line with the naming around.

Comment 8

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Comment on attachment 9027544 [details] [diff] [review]
> bug1508665-split-basestackframe.patch
> 
> Drawing the different sections in ASCII art would be
> really nice to have a very fast intuition of how localSize(), fixedSize(),
> InitialChunk, currentStackHeight() and masm.framePushed() relate to each
> other.

I will try; it gets a little messy, in 80 columns...

> @@ +1221,5 @@
> > +
> > +    // Record the current stack height, after it has become stable in
> > +    // beginFunction().  See also BaseStackFrame::onFixedStackAllocated().
> > +
> > +    void onFixedStackAllocated() {
> 
> Should this function just be merged with setLocalSize? If I understand
> correctly the local size can't change between the setLocalSize() and the
> onFixedStackAllocated() calls.

Right.  That's the way it was before and I decided it was too confusing, and so I have split them, so that there is a clear point in time where we tell everyone that the fixed stack area has been allocated, I found this more intuitive.  It's probable that there is no good solution here :(

> @@ +1256,5 @@
> > +    }
> > +
> > +#ifdef RABALDR_CHUNKY_STACK
> > +
> > +# define CHUNKY_INVARIANT()                                          \
> 
> Could it just be an inline function instead? (no need to make it explicitly
> inlined as it'd be defined in the class definition)

Paranoia on my part about the "inline" function always being inlined reliably by every compiler on every platform...  and a wish not to have a call there to affect optimization locally.  Probably too much paranoia.  I'm still thinking about this, not sure what I want to do.  I see your point.

> @@ +1264,5 @@
> > +
> > +    void pushChunkyBytes(uint32_t bytes) {
> > +        CHUNKY_INVARIANT();
> > +        if (masm.framePushed() - currentStackHeight_ < bytes) {
> > +            masm.reserveStack(ChunkSize);
> 
> Just noticed: could it fail if bytes > (masm.framePushed() -
> currentStackHeight_ + ChunkSize)? I guess it would trigger the chunk
> invariant asserts below, so it would happen that none of the pushes in the
> baseline compiler are big. If I'm not misunderstood, might be nice to either
> generalize this code so it handles bigger bytes size, or add a comment that
> it works because we only do small pushes? (or MOZ_ASSERT bytes <= ChunkSize
> as a stricter assertion)

You're right.  It's an invariant that all pushes are "small", and there should be an assert for that.  cf the comparable code in pop, that allows larger sections to be popped.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d11eb20a6af
Split BaseStackFrame to simplify it.  r=bbouvier

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0d11eb20a6af