Bug 215114 - CheckpointSideState shoud play nicely with StackOverflowException unwinding.
Summary: CheckpointSideState shoud play nicely with StackOverflowException unwinding.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-03 17:04 PDT by Keith Miller
Modified: 2020-08-04 18:45 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.47 KB, patch)
2020-08-03 17:23 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (21.84 KB, patch)
2020-08-04 17:41 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-08-03 17:04:40 PDT
CheckpointSideState shoud play nicely with StackOverflowException unwinding.
Comment 1 Keith Miller 2020-08-03 17:23:57 PDT
Created attachment 405892 [details]
Patch
Comment 2 Saam Barati 2020-08-03 17:45:50 PDT
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
Comment 3 Mark Lam 2020-08-03 17:59:14 PDT
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/
Comment 4 Mark Lam 2020-08-03 18:01:53 PDT
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"?
Comment 5 Mark Lam 2020-08-03 18:03:00 PDT
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?
Comment 6 Saam Barati 2020-08-03 18:05:58 PDT
(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.
Comment 7 Mark Lam 2020-08-03 18:07:44 PDT
(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.
Comment 8 Keith Miller 2020-08-04 12:03:20 PDT
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.
Comment 9 Keith Miller 2020-08-04 17:01:26 PDT
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.
Comment 10 Keith Miller 2020-08-04 17:41:12 PDT
Created attachment 405969 [details]
Patch for landing
Comment 11 EWS 2020-08-04 18:32:56 PDT
Committed r265272: <https://trac.webkit.org/changeset/265272>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405969 [details].
Comment 12 Radar WebKit Bug Importer 2020-08-04 18:33:16 PDT
<rdar://problem/66552728>
Comment 13 Saam Barati 2020-08-04 18:45:35 PDT
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.