Bug 215114 – CheckpointSideState shoud play nicely with StackOverflowException unwinding.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

CheckpointSideState shoud play nicely with StackOverflowException unwinding.

[Keith Miller]

Created attachment 405892 [details]
Patch

[Saam Barati]

Comment on attachment 405892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405892&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        automatically unwind into the first frame before calling into the

you should say why: we do this b/c stack overflow does it purposefully because the top frame isn't fully materialized.

> Source/JavaScriptCore/ChangeLog:16
> +        miggrated us from another thread below the current thread.

miggrated -> migrated

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:663
> +

revert

> Source/JavaScriptCore/runtime/VM.cpp:1058
> +#if ASSERT_ENABLED
> +    for (const auto& sideState : m_checkpointSideState)
> +        ASSERT(sideState->associatedCallFrame != callFrame);
> +#endif

Let's also assert it's sorted after the append.

> Source/JavaScriptCore/runtime/VM.cpp:1066
> +    ASSERT_UNUSED(expectedCallFrame, sideState->associatedCallFrame == expectedCallFrame);

let's make this RELEASE_ASSERT

> Source/JavaScriptCore/runtime/VM.cpp:1074
> +    ASSERT(bounds.contains(target));

WDYT about a release assert?

> Source/WTF/wtf/StackBounds.h:107
> +    StackBounds withSoftOrigin(void* origin) const

nit: Why not call this "withOrigin"? Seems more in line with the name "origin()" used elsewhere

[Mark Lam]

Comment on attachment 405892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405892&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        This patch fixes an issue where we the StackWalker would

Let's call it what it's called in code: /StackWalker/StackVisitor/.

> Source/JavaScriptCore/ChangeLog:21
> +        with it is now marked const so we can PointerDump a CallFrame*.

What does PointerDump mean?  Do you mean dataLog?

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:663
>> +
> 
> revert

Unnecessary blank line.

> Source/JavaScriptCore/runtime/VM.cpp:1054
> +    payload->associatedCallFrame = callFrame;

Since every payload must have an associatedCallFrame, can you just make this initialization part of the constructor?  That would be a more robust idiom.

> Source/JavaScriptCore/runtime/VM.cpp:1076
> +    // We have to worry about migrating from another thread since there may be no chehckpoints in our thread but one in the other threads.

/chehckpoints/checkpoints/
/but one in the other/but have one in the other/

[Mark Lam]

Comment on attachment 405892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405892&action=review

> Source/JavaScriptCore/runtime/VM.cpp:1070
> +void VM::popAllCheckpointOSRSideStateUntil(CallFrame* target)

nit: maybe call this "popAllCheckpointOSRSideStateUntilAndIncluding"?

[Mark Lam]

Comment on attachment 405892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405892&action=review

> Source/JavaScriptCore/runtime/VM.h:1209
> +    Vector<std::unique_ptr<CheckpointOSRExitSideState>, 4> m_checkpointSideState;

How did you decide on this depth of 4?  Is it guaranteed to always be enough?

[Saam Barati]

(In reply to Mark Lam from comment #5)
> Comment on attachment 405892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405892&action=review
> 
> > Source/JavaScriptCore/runtime/VM.h:1209
> > +    Vector<std::unique_ptr<CheckpointOSRExitSideState>, 4> m_checkpointSideState;
> 
> How did you decide on this depth of 4?  Is it guaranteed to always be enough?

No, this is a dynamic value. This is an inline capacity.

[Mark Lam]

(In reply to Saam Barati from comment #6)
> (In reply to Mark Lam from comment #5)
> > Comment on attachment 405892 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=405892&action=review
> > 
> > > Source/JavaScriptCore/runtime/VM.h:1209
> > > +    Vector<std::unique_ptr<CheckpointOSRExitSideState>, 4> m_checkpointSideState;
> > 
> > How did you decide on this depth of 4?  Is it guaranteed to always be enough?
> 
> No, this is a dynamic value. This is an inline capacity.

Oh, right.  Ignore me.

[Keith Miller]

Comment on attachment 405892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405892&action=review

Looking into the microbenchmark failures now.

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch fixes an issue where we the StackWalker would
> 
> Let's call it what it's called in code: /StackWalker/StackVisitor/.

Yeah, whoops, dunno why I wrote StackWalker...

>> Source/JavaScriptCore/ChangeLog:9
>> +        automatically unwind into the first frame before calling into the
> 
> you should say why: we do this b/c stack overflow does it purposefully because the top frame isn't fully materialized.

Sure.

>> Source/JavaScriptCore/ChangeLog:16
>> +        miggrated us from another thread below the current thread.
> 
> miggrated -> migrated

fixed.

>> Source/JavaScriptCore/ChangeLog:21
>> +        with it is now marked const so we can PointerDump a CallFrame*.
> 
> What does PointerDump mean?  Do you mean dataLog?

It's a class that dumps your pointers in DataLog.

>> Source/JavaScriptCore/runtime/VM.cpp:1054
>> +    payload->associatedCallFrame = callFrame;
> 
> Since every payload must have an associatedCallFrame, can you just make this initialization part of the constructor?  That would be a more robust idiom.

I'm not sure that's more robust because the only person that cares about that is VM for the clearing below. I kept it this way so it's more or of a HashMap style usage. The fact that it's a stack under the hood shouldn't matter. This way if for whatever reason we need to switch back to a HashMap we can do that without touching clients.

>> Source/JavaScriptCore/runtime/VM.cpp:1058
>> +#endif
> 
> Let's also assert it's sorted after the append.

That's not necessarily true because of stack switching. I guess you could be sorted as long as your in the same stack but that's getting complicated...

>> Source/JavaScriptCore/runtime/VM.cpp:1066
>> +    ASSERT_UNUSED(expectedCallFrame, sideState->associatedCallFrame == expectedCallFrame);
> 
> let's make this RELEASE_ASSERT

Sure.

>> Source/JavaScriptCore/runtime/VM.cpp:1070
>> +void VM::popAllCheckpointOSRSideStateUntil(CallFrame* target)
> 
> nit: maybe call this "popAllCheckpointOSRSideStateUntilAndIncluding"?

I think of until as inclusive and before as exclusive...

>> Source/JavaScriptCore/runtime/VM.cpp:1074
>> +    ASSERT(bounds.contains(target));
> 
> WDYT about a release assert?

Don't think it matters because the compiler will remove the assert if it's true.

>> Source/JavaScriptCore/runtime/VM.cpp:1076
>> +    // We have to worry about migrating from another thread since there may be no chehckpoints in our thread but one in the other threads.
> 
> /chehckpoints/checkpoints/
> /but one in the other/but have one in the other/

I need spell check for comments. lol

>>>> Source/JavaScriptCore/runtime/VM.h:1209
>>>> +    Vector<std::unique_ptr<CheckpointOSRExitSideState>, 4> m_checkpointSideState;
>>> 
>>> How did you decide on this depth of 4?  Is it guaranteed to always be enough?
>> 
>> No, this is a dynamic value. This is an inline capacity.
> 
> Oh, right.  Ignore me.

Yeah, I just picked it because checkpoints are rare but you may hit them often enough during repeated OSR exiting it'd be good to avoid pathological malloc/free.

>> Source/WTF/wtf/StackBounds.h:107
>> +    StackBounds withSoftOrigin(void* origin) const
> 
> nit: Why not call this "withOrigin"? Seems more in line with the name "origin()" used elsewhere

I was thinking soft because it's not the real stack bound and it should be within the current StackBounds. But I don't feel strongly about it.

[Keith Miller]

Comment on attachment 405892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405892&action=review

>>> Source/JavaScriptCore/runtime/VM.cpp:1054
>>> +    payload->associatedCallFrame = callFrame;
>> 
>> Since every payload must have an associatedCallFrame, can you just make this initialization part of the constructor?  That would be a more robust idiom.
> 
> I'm not sure that's more robust because the only person that cares about that is VM for the clearing below. I kept it this way so it's more or of a HashMap style usage. The fact that it's a stack under the hood shouldn't matter. This way if for whatever reason we need to switch back to a HashMap we can do that without touching clients.

Nvm, I fixed a bug where I forgot to flip the inline stack before pushing it. Once you do that it's more annoying to have the frame not part of the CheckpointOSRExitSideState constructor.

[Keith Miller]

Created attachment 405969 [details]
Patch for landing

[EWS]

Committed r265272: <https://trac.webkit.org/changeset/265272>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405969 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66552728>

[Saam Barati]

Comment on attachment 405969 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=405969&action=review

> Source/JavaScriptCore/runtime/VM.cpp:1051
> +void VM::pushCheckpointOSRSideState(std::unique_ptr<CheckpointOSRExitSideState>&& payload)

we should assert at the end of this function that the side stack is sorted.