| Summary: | CheckpointSideState shoud play nicely with StackOverflowException unwinding. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Keith Miller
2020-08-03 17:04:40 PDT
Created attachment 405892 [details]
Patch
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 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 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 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? (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. (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 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 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. Created attachment 405969 [details]
Patch for landing
Committed r265272: <https://trac.webkit.org/changeset/265272> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405969 [details]. 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. |