1194451 - Avoid copying frames in adoptAsyncStack for async stacks

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Async stacks are expensive right now (bug 1152893) because when we capture a stack when there is an async parent stack available, we do not simply use the available async stack as the async parent of the newly captured stack. In order to avoid async stack chains that are unbounded in length, we use adoptAsyncStack to copy a bounded number of frames into a new SavedFrame object stack which is then used for the async parent of the newly captured stack.

We do that copying to keep from having an unbounded number of async parents and making it so they can never be collected by the GC.

If we could figure out a different way to avoid having an unbounded number of async parents, we could avoid the expensive copying in adoptAsyncStack (at least in the case of same compartment async stacks).

My suggestion: do not add async parents to stacks that are captured for use as an async parent. This would limit all stacks to a single async parent. Then, when capturing new stacks with this async parent, we could just set the async parent directly rather than copying the first n frames since we know that the number of async parent frames is bounded.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

AutoSetAsyncStackForNewCalls would assert that the frame that it is given does not have an async parent in debug builds.

JS::CaptureCurrentStack would have a mode for ignoring any async parent that might exist.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The idea is that this way the only additional work that happens when we are capturing stacks with an async parent vs when we are not is the setting of the async parent pointer.

(Assuming same compartments between async parent and cx's current compartment)

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Actually, you would have to copy/rewrite the youngest frame of the async parent stack to include the cause. However this is copying 1 frame vs n frames (now). Seems like it should still be a big win :)

Comment 4

[:Paolo Amadini]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #0)
> This would limit all stacks to a single async parent.

I have a feeling that this would be pretty limiting for Promise "then" chains...

If the stacks are limited to an arbitrary length rather than a given number of frames anyways, I guess the solution I proposed in bug 1177508 comment 4 would be a comparably high performance win. That's very easy to implement and I may finally get to it as I have to do some platform work in the next weeks anyways (though you're more than welcome to help!).

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to :Paolo Amadini from comment #4)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #0)
> > This would limit all stacks to a single async parent.
> 
> I have a feeling that this would be pretty limiting for Promise "then"
> chains...

One async parent is >>>>>>>> than zero async parents, which is what we are shipping now. Is it all the context? No, we can't have all the context (which is the whole reason why we are doing adoptAsyncStack / limiting frames / etc). Is it enough context to be useful and provide value? Yes, definitely.

> If the stacks are limited to an arbitrary length rather than a given number
> of frames anyways, I guess the solution I proposed in bug 1177508 comment 4
> would be a comparably high performance win. That's very easy to implement
> and I may finally get to it as I have to do some platform work in the next
> weeks anyways (though you're more than welcome to help!).

Even if you limit to an arbitrary number of stack frames, you still have to do the copying as soon as you cross an async parent edge. For example, if you have 30 frames in the async parent, and a limit of relatively little async frames, say 10, then you still have to copy/rewrite 10 frames from that async parent stack. If you are doing the method described in this bug, you can get all 30 frames and you only have to copy/rewrite a single frame (the youngest frame of the async parent stack).

Copying is too expensive, this method is a clear win.