Bug 164037 – [JSC] Handle new_async_func / new_async_func_exp in DFG / FTL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caitlin Potter (:caitp)]

[JSC] Handle new_async_func / new_async_func_exp in DFG / FTL

[Caitlin Potter (:caitp)]

Created attachment 292964 [details]
Patch

Add DFG/FTL code for async functions. Blocked on https://bugs.webkit.org/show_bug.cgi?id=163760

[Caitlin Potter (:caitp)]

Created attachment 293174 [details]
Patch

[Caitlin Potter (:caitp)]

Yusuke in particular (but also anyone else), how does this look? (It's been sitting here a few days :).

Obviously this won't help any with the parser regressions when the flag is enabled, hopefully I can take a look at those sometime this week.

[Saam Barati]

Comment on attachment 293174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293174&action=review

> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());

Why is this safe to do? Seems like this could return null?

[Caitlin Potter (:caitp)]

Comment on attachment 293174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293174&action=review

>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
>> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());
> 
> Why is this safe to do? Seems like this could return null?

I believe the lazy structure is guaranteed to be initialized before optimization of it, by the baseline version.

[Saam Barati]

(In reply to comment #5)
> Comment on attachment 293174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293174&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
> >> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());
> > 
> > Why is this safe to do? Seems like this could return null?
> 
> I believe the lazy structure is guaranteed to be initialized before
> optimization of it, by the baseline version.

Why is that? What if the code never executes?

[Caitlin Potter (:caitp)]

Comment on attachment 293174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293174&action=review

>>>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
>>>> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());
>>> 
>>> Why is this safe to do? Seems like this could return null?
>> 
>> I believe the lazy structure is guaranteed to be initialized before optimization of it, by the baseline version.
> 
> Why is that? What if the code never executes?

Is there a situation where FTL graphs can be generated before baseline bytecode? If that is possible, I think we will want to test for that.

[Yusuke Suzuki]

(In reply to comment #7)
> Comment on attachment 293174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293174&action=review
> 
> >>>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
> >>>> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());
> >>> 
> >>> Why is this safe to do? Seems like this could return null?
> >> 
> >> I believe the lazy structure is guaranteed to be initialized before optimization of it, by the baseline version.
> > 
> > Why is that? What if the code never executes?
> 
> Is there a situation where FTL graphs can be generated before baseline
> bytecode? If that is possible, I think we will want to test for that.

Yes, the following script crashes.

```
function test(cond)
{
    if (cond) {
        var func = async function () {
        };
        return func;
    }
    return 42;
}
noInline(test);

for (var i = 0; i < 1e4; ++i)
    test();
```

I don't think we should rely on that. We easily miss the case.
I think non lazy thing should be better in this case.

[Caitlin Potter (:caitp)]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 293174 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=293174&action=review
> > 
> > >>>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
> > >>>> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());
> > >>> 
> > >>> Why is this safe to do? Seems like this could return null?
> > >> 
> > >> I believe the lazy structure is guaranteed to be initialized before optimization of it, by the baseline version.
> > > 
> > > Why is that? What if the code never executes?
> > 
> > Is there a situation where FTL graphs can be generated before baseline
> > bytecode? If that is possible, I think we will want to test for that.
> 
> Yes, the following script crashes.
> 
> ```
> function test(cond)
> {
>     if (cond) {
>         var func = async function () {
>         };
>         return func;
>     }
>     return 42;
> }
> noInline(test);
> 
> for (var i = 0; i < 1e4; ++i)
>     test();
> ```
> 
> I don't think we should rely on that. We easily miss the case.
> I think non lazy thing should be better in this case.

The main benefit of the lazy structure is compiling the asyncFunctionResume builtin lazily. Unfortunately, it seems to be impossible to use an ordinary LazyPeoperty for this, currently.

Building this builtin on code which doesn't need it (e.g all benchmarks) results in regressions due to the increased startup time.

[Saam Barati]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 293174 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=293174&action=review
> > > 
> > > >>>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:157
> > > >>>> +                    registerStructure(m_graph.globalObjectFor(node->origin.semantic)->asyncFunctionStructureConcurrently());
> > > >>> 
> > > >>> Why is this safe to do? Seems like this could return null?
> > > >> 
> > > >> I believe the lazy structure is guaranteed to be initialized before optimization of it, by the baseline version.
> > > > 
> > > > Why is that? What if the code never executes?
> > > 
> > > Is there a situation where FTL graphs can be generated before baseline
> > > bytecode? If that is possible, I think we will want to test for that.
> > 
> > Yes, the following script crashes.
> > 
> > ```
> > function test(cond)
> > {
> >     if (cond) {
> >         var func = async function () {
> >         };
> >         return func;
> >     }
> >     return 42;
> > }
> > noInline(test);
> > 
> > for (var i = 0; i < 1e4; ++i)
> >     test();
> > ```
> > 
> > I don't think we should rely on that. We easily miss the case.
> > I think non lazy thing should be better in this case.
> 
> The main benefit of the lazy structure is compiling the asyncFunctionResume
> builtin lazily. Unfortunately, it seems to be impossible to use an ordinary
> LazyPeoperty for this, currently.
> 
> Building this builtin on code which doesn't need it (e.g all benchmarks)
> results in regressions due to the increased startup time.
This surprises me that this pushes us over on some benchmarks and is noticeable. What benchmarks are you seeing that compiling the builtin causes a noticeable slowdown?

[Caitlin Potter (:caitp)]

Created attachment 294725 [details]
Patch

[Caitlin Potter (:caitp)]

In the newest patchset, the lazy class structure is no longer used (might be worth lazily compiling the builtin, to avoid apparent benchmark regressions). I've added the test case from comment 8 to the test suite.

[Yusuke Suzuki]

(In reply to comment #12)
> In the newest patchset, the lazy class structure is no longer used (might be
> worth lazily compiling the builtin, to avoid apparent benchmark
> regressions). I've added the test case from comment 8 to the test suite.

Nice! Could you paste performance numbers?
I would like to know the impact of that. I think it is small.

[Caitlin Potter (:caitp)]

(In reply to comment #13)
> (In reply to comment #12)
> > In the newest patchset, the lazy class structure is no longer used (might be
> > worth lazily compiling the builtin, to avoid apparent benchmark
> > regressions). I've added the test case from comment 8 to the test suite.
> 
> Nice! Could you paste performance numbers?
> I would like to know the impact of that. I think it is small.

once I have numbers to share, I'll paste them here.

[Caitlin Potter (:caitp)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > In the newest patchset, the lazy class structure is no longer used (might be
> > > worth lazily compiling the builtin, to avoid apparent benchmark
> > > regressions). I've added the test case from comment 8 to the test suite.
> > 
> > Nice! Could you paste performance numbers?
> > I would like to know the impact of that. I think it is small.
> 
> once I have numbers to share, I'll paste them here.

Default octane run:

```
VMs tested:
"WithoutPatch" at /Users/caitp/git/WebKit/WebKitBuild/WithoutPatch/JavaScriptCore.framework/Versions/A/Resources/jsc
"WithPatch" at /Users/caitp/git/WebKit/WebKitBuild/WithPatch/JavaScriptCore.framework/Versions/A/Resources/jsc

Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution
times with 95% confidence intervals in milliseconds.

                          WithoutPatch               WithPatch                                     

encrypt                 0.15126+-0.00541          0.14842+-0.00176         might be 1.0191x faster
decrypt                 2.60415+-0.12859          2.55074+-0.04184         might be 1.0209x faster
deltablue      x2       0.12590+-0.01974          0.12304+-0.00420         might be 1.0232x faster
earley                  0.23794+-0.00347          0.23679+-0.00243       
boyer                   4.04547+-0.03166    ^     3.92693+-0.05468       ^ definitely 1.0302x faster
navier-stokes  x2       4.60507+-0.05944          4.60082+-0.11198       
raytrace       x2       0.65420+-0.00899          0.65410+-0.01121       
richards       x2       0.07991+-0.00167          0.07941+-0.00256       
splay          x2       0.31743+-0.00703          0.31308+-0.00730         might be 1.0139x faster
regexp         x2      17.01311+-1.02205    ?    17.30320+-2.03705       ? might be 1.0171x slower
pdfjs          x2      39.98251+-1.24177         39.69165+-1.15526       
mandreel       x2      40.69541+-1.01094         39.68283+-0.88594         might be 1.0255x faster
gbemu          x2      28.75390+-0.28842         28.55844+-0.48394       
closure                 0.47163+-0.00477          0.46989+-0.01574       
jquery                  6.49217+-0.68149          6.33684+-0.14653         might be 1.0245x faster
box2d          x2       8.69406+-0.12519    ?     8.77782+-0.07552       ?
zlib           x2     329.95036+-17.57603   ?   334.79163+-19.78330      ? might be 1.0147x slower
typescript     x2     603.51654+-8.96355    ?   613.43915+-20.67173      ? might be 1.0164x slower

<geometric>             4.75141+-0.04543          4.72779+-0.04072         might be 1.0050x faster
```

I expect that the increased startup time plays a role, but it could be a relatively small role in practice.

[Yusuke Suzuki]

Comment on attachment 294725 [details]
Patch

r=me. Perf numbers are neutral.
It's time to open a bug enabling async/await!

[Caitlin Potter (:caitp)]

(In reply to comment #16)
> Comment on attachment 294725 [details]
> Patch
> 
> r=me. Perf numbers are neutral.
> It's time to open a bug enabling async/await!

Still have some work to do on the frontend performance :\ I've been working on that today, but don't have a patch ready yet.

[Yusuke Suzuki]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 294725 [details]
> > Patch
> > 
> > r=me. Perf numbers are neutral.
> > It's time to open a bug enabling async/await!
> 
> Still have some work to do on the frontend performance :\ I've been working
> on that today, but don't have a patch ready yet.

:\, yeah, anyway, you can just open a bug for enabling async/await things :) (w/o patch)

[WebKit Commit Bot]

Comment on attachment 294725 [details]
Patch

Clearing flags on attachment: 294725

Committed r208704: <http://trac.webkit.org/changeset/208704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Caitlin Potter (:caitp)]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Comment on attachment 294725 [details]
> > > Patch
> > > 
> > > r=me. Perf numbers are neutral.
> > > It's time to open a bug enabling async/await!
> > 
> > Still have some work to do on the frontend performance :\ I've been working
> > on that today, but don't have a patch ready yet.
> 
> :\, yeah, anyway, you can just open a bug for enabling async/await things :)
> (w/o patch)

I think https://bugs.chromium.org/p/v8/issues/detail?id=5390 is a blocker for shipping as well. Currently, we have a similar leak where each intermediate Promise is retained when ideally it should be discarded when it's no longer needed, creating a memory leak for these sorts of loops.

[Yusuke Suzuki]

(In reply to comment #21)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > Comment on attachment 294725 [details]
> > > > Patch
> > > > 
> > > > r=me. Perf numbers are neutral.
> > > > It's time to open a bug enabling async/await!
> > > 
> > > Still have some work to do on the frontend performance :\ I've been working
> > > on that today, but don't have a patch ready yet.
> > 
> > :\, yeah, anyway, you can just open a bug for enabling async/await things :)
> > (w/o patch)
> 
> I think https://bugs.chromium.org/p/v8/issues/detail?id=5390 is a blocker
> for shipping as well. Currently, we have a similar leak where each
> intermediate Promise is retained when ideally it should be discarded when
> it's no longer needed, creating a memory leak for these sorts of loops.

Interesting. It seems that we can just split the promise chain to solve this issue, right?

[Caitlin Potter (:caitp)]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > (In reply to comment #16)
> > > > > Comment on attachment 294725 [details]
> > > > > Patch
> > > > > 
> > > > > r=me. Perf numbers are neutral.
> > > > > It's time to open a bug enabling async/await!
> > > > 
> > > > Still have some work to do on the frontend performance :\ I've been working
> > > > on that today, but don't have a patch ready yet.
> > > 
> > > :\, yeah, anyway, you can just open a bug for enabling async/await things :)
> > > (w/o patch)
> > 
> > I think https://bugs.chromium.org/p/v8/issues/detail?id=5390 is a blocker
> > for shipping as well. Currently, we have a similar leak where each
> > intermediate Promise is retained when ideally it should be discarded when
> > it's no longer needed, creating a memory leak for these sorts of loops.
> 
> Interesting. It seems that we can just split the promise chain to solve this
> issue, right?

I'm working on a fix for this, I will probably send it tonight.

[Yusuke Suzuki]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #18)
> > > > (In reply to comment #17)
> > > > > (In reply to comment #16)
> > > > > > Comment on attachment 294725 [details]
> > > > > > Patch
> > > > > > 
> > > > > > r=me. Perf numbers are neutral.
> > > > > > It's time to open a bug enabling async/await!
> > > > > 
> > > > > Still have some work to do on the frontend performance :\ I've been working
> > > > > on that today, but don't have a patch ready yet.
> > > > 
> > > > :\, yeah, anyway, you can just open a bug for enabling async/await things :)
> > > > (w/o patch)
> > > 
> > > I think https://bugs.chromium.org/p/v8/issues/detail?id=5390 is a blocker
> > > for shipping as well. Currently, we have a similar leak where each
> > > intermediate Promise is retained when ideally it should be discarded when
> > > it's no longer needed, creating a memory leak for these sorts of loops.
> > 
> > Interesting. It seems that we can just split the promise chain to solve this
> > issue, right?
> 
> I'm working on a fix for this, I will probably send it tonight.

Cool!