[PRO-901] Add DG adoptedPromise edge
#264
Conversation
Domiii
commented
Nov 19, 2024
•
Domiii
commented
- https://linear.app/replay/issue/PRO-901/add-nested-persistent-id-to-promisesettled-nodes
- paired w/
- DEPS (v8) chromium#1311
Domiii
changed the title
nested_persistent_idadoptedPromise edge
| if (RecordReplayShouldCallOnPromiseHook() && reason->IsJSReceiver()) { | ||
| AddPromiseDependencyGraphAdoption(isolate, Handle<Object>::cast(promise), reason); | ||
| } |
There was a problem hiding this comment.
Is this needed? I'm not sure if my initial conclusion that it would be holds true 😅
| @@ -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); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
