Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[PRO-901] Add DG adoptedPromise edge #264

Merged
merged 4 commits into from
Nov 20, 2024
+23 −0
Merged

[PRO-901] Add DG adoptedPromise edge #264

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dab820f
Add DG `nested_persistent_id`
Domiii Nov 19, 2024
72d0f1f
Refactor proposal with adoption edges
Andarist Nov 19, 2024
3f78b5a
Adoption fixes
Domiii Nov 19, 2024
1143728
mini data change
Domiii Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/debug/debug.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4261,6 +4261,17 @@ GetOrCreatePromiseDependencyGraphData(Isolate* isolate, Handle<Object> promise)
return iter->second;
}

void AddPromiseDependencyGraphAdoption(Isolate* isolate, Handle<Object> promise, Handle<Object> adopted) {
PromiseDependencyGraphData& data = GetOrCreatePromiseDependencyGraphData(isolate, promise);
PromiseDependencyGraphData& adopted_data = GetOrCreatePromiseDependencyGraphData(isolate, adopted);
Copy link
Member

@Andarist Andarist Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might, slightly, dilute what GetOrCreatePromiseDependencyGraphData is about. By reading this code I'd think it would contain only promise IDs but that's not quite true as other objects could potentially be "registered" in it, right?

I also wonder if this whole thing can now generate a sensible graph when thenables are involved

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with these concerns, GetOrCreatePromiseDependencyGraphData should only be used with promises. Things won't break if it's used with other objects, but the edges being created here seem fairly meaningless. I'd like a more detailed explanation of how this PR helps to be able to review it.


if (adopted_data.new_node_id) {
recordreplay::AddDependencyGraphEdge(data.new_node_id,
adopted_data.new_node_id,
"{\"kind\":\"adoptedPromise\"}");
}
}

extern bool gRecordReplayEnableDependencyGraph;

bool RecordReplayShouldCallOnPromiseHook() {
Expand Down
12 changes: 12 additions & 0 deletions src/objects/objects.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5470,6 +5470,10 @@ void JSPromise::set_async_task_id(int id) {
set_flags(AsyncTaskIdBits::update(flags(), id));
}


extern bool RecordReplayShouldCallOnPromiseHook();
void AddPromiseDependencyGraphAdoption(Isolate* isolate, Handle<Object> promise, Handle<Object> adoption);

// static
Handle<Object> JSPromise::Fulfill(Handle<JSPromise> promise,
Handle<Object> value) {
Expand Down Expand Up @@ -5550,6 +5554,10 @@ Handle<Object> JSPromise::Reject(Handle<JSPromise> promise,
kPromiseRejectWithNoHandler);
}

if (RecordReplayShouldCallOnPromiseHook() && reason->IsJSReceiver()) {
AddPromiseDependencyGraphAdoption(isolate, Handle<Object>::cast(promise), reason);
}
Comment on lines +5557 to +5559
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? I'm not sure if my initial conclusion that it would be holds true 😅

Copy link
Author

@Domiii Domiii Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Its not. Rejections indeed cannot nest promises. I somehow thought they could, but I'm definitely wrong.
For example, here, A does not delay B:

var p = Promise.resolve().then(() => new Promise(r => setTimeout(r,100))).then(() => console.log("A"));
Promise.reject(p).finally(() => console.log("B"))


// 8. Return TriggerPromiseReactions(reactions, reason).
return TriggerPromiseReactions(isolate, reactions, reason,
PromiseReaction::kReject);
Expand Down Expand Up @@ -5625,6 +5633,10 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
return Fulfill(promise, resolution);
}

if (RecordReplayShouldCallOnPromiseHook()) {
AddPromiseDependencyGraphAdoption(isolate, Handle<Object>::cast(promise), resolution);
}

// 13. Let job be NewPromiseResolveThenableJob(promise, resolution,
// thenAction).
Handle<NativeContext> then_context;
Expand Down