-
Notifications
You must be signed in to change notification settings - Fork 5k
jsc: drain rejected-promise list in O(n) instead of O(n^2) #32554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8fa8c26
2b9af97
1930ad4
8aec644
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1053,6 +1053,18 @@ void GlobalObject::promiseRejectionTracker(JSGlobalObject* obj, JSC::JSPromise* | |
| return unhandledPromise.get() == promise; | ||
| }); | ||
| if (removed) break; | ||
| // handleRejectedPromises() drains the list into a local buffer before | ||
| // running any handler. A handler may .catch() a later still-queued | ||
| // promise; that promise is no longer in m_aboutToBeNotifiedRejectedPromises | ||
| // but has not yet had 'unhandledRejection' fired, so it must not get | ||
| // 'rejectionHandled'. Check every in-flight tail (handlers can re-enter | ||
| // handleRejectedPromises(), so there may be more than one). | ||
| for (auto* inflight = globalObj->m_rejectedPromisesBeingProcessed; inflight; inflight = inflight->outer) { | ||
| for (size_t i = inflight->index, n = inflight->buffer->size(); i < n; ++i) { | ||
| if (inflight->buffer->at(i).asCell() == promise) | ||
| return; | ||
| } | ||
| } | ||
| // The promise rejection has already been notified, now we need to queue it for the rejectionHandled event | ||
| Bun__handleHandledPromise(globalObj, promise); | ||
| break; | ||
|
|
@@ -3278,18 +3290,41 @@ extern "C" void Bun__handleRejectedPromise(Zig::GlobalObject* JSGlobalObject, JS | |
|
|
||
| void GlobalObject::handleRejectedPromises() | ||
| { | ||
| if (m_aboutToBeNotifiedRejectedPromises.isEmpty()) [[likely]] | ||
| return; | ||
|
|
||
| JSC::VM& virtual_machine = vm(); | ||
| auto scope = DECLARE_TOP_EXCEPTION_SCOPE(virtual_machine); | ||
| while (auto* promise = m_aboutToBeNotifiedRejectedPromises.takeFirst(this)) { | ||
| if (promise->isHandled()) | ||
| continue; | ||
|
|
||
| Bun__handleRejectedPromise(this, promise); | ||
| if (auto ex = scope.exception()) { | ||
| (void)scope.tryClearException(); | ||
| this->reportUncaughtExceptionAtEventLoop(this, ex); | ||
| do { | ||
| // Move the whole list out under one cellLock, then iterate linearly — | ||
| // the same pattern JSC's VM::didExhaustMicrotaskQueue and WebCore's | ||
| // RejectedPromiseTracker use. | ||
| JSC::MarkedArgumentBuffer promises; | ||
| m_aboutToBeNotifiedRejectedPromises.drainTo(this, promises); | ||
| RELEASE_ASSERT(!promises.hasOverflowed()); | ||
| // Expose the not-yet-processed tail so promiseRejectionTracker(Handle) | ||
| // can tell "still pending" apart from "already notified". Linked as a | ||
| // stack so a re-entrant handleRejectedPromises() (a handler that ticks | ||
| // the event loop) restores the outer frame instead of nulling it. | ||
| InFlightRejections inflight { &promises, 0, m_rejectedPromisesBeingProcessed }; | ||
| WTF::SetForScope inflightScope(m_rejectedPromisesBeingProcessed, &inflight); | ||
| for (size_t i = 0, size = promises.size(); i < size; ++i) { | ||
| auto* promise = static_cast<JSC::JSPromise*>(promises.at(i).asCell()); | ||
| if (promise->isHandled()) | ||
| continue; | ||
|
claude[bot] marked this conversation as resolved.
|
||
| inflight.index = i + 1; | ||
|
|
||
| Bun__handleRejectedPromise(this, promise); | ||
| if (auto ex = scope.exception()) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if its a termination exception? tryClearException will return false and what we should do, in that case, is return out of this function.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @robobun can you resume this PR
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 8aec644: checks |
||
| if (virtual_machine.isTerminationException(ex)) [[unlikely]] | ||
| return; | ||
| (void)scope.tryClearException(); | ||
| this->reportUncaughtExceptionAtEventLoop(this, ex); | ||
| } | ||
| } | ||
| } | ||
| // An unhandledRejection handler may itself reject a promise; loop | ||
| // until the list stays empty. | ||
| } while (!m_aboutToBeNotifiedRejectedPromises.isEmpty()); | ||
| } | ||
|
|
||
| DEFINE_VISIT_CHILDREN(GlobalObject); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.