Updating async function implementation to fix the symbol capturing issues #1456

aneeshdk · 2016-08-17T17:22:16Z

Updating async function implementation by desugaring to generators

This change removes the function wrapper implementation of async
functions. Now the async functions will behave similar to generator
functions. The actual function body will be in the sciptFunction field.
Symbols now need not be forced to slots. asyncspawn opcode and parse
node are no more needed.In the backend async functions are also
added to the generator JIT flag. await is desugared into yield.

aneeshdk · 2016-08-17T17:34:23Z

@ianwjhalliday @pleath @akroshg Can you please take a look?

ianwjhalliday · 2016-08-17T21:27:28Z

lib/Backend/Func.cpp

@@ -846,8 +846,8 @@ Func::AjustLocalVarSlotOffset()
 bool
 Func::DoGlobOptsForGeneratorFunc()
 {
-    // Disable GlobOpt optimizations for generators initially. Will visit and enable each one by one.
-    return !m_jnFunction->IsGenerator();
+    // Disable GlobOpt optimizations for generators and asyn functions initially. Will visit and enable each one by one.


asyn [](start = 56, length = 4)

typo

ianwjhalliday · 2016-08-18T01:19:03Z

                sym->GetScope()->GetFunc()->root->sxFnc.IsAsync());

I think this IsAsync check should be removed from this assert now

Refers to: lib/Runtime/ByteCode/ByteCodeGenerator.cpp:2434 in 8727492. [](commit_id = 8727492, deletion_comment = False)

ianwjhalliday · 2016-08-18T01:19:23Z

            if (parentFunc->root->sxFnc.IsAsync())

This should be removed, no longer true

Refers to: lib/Runtime/ByteCode/ByteCodeGenerator.cpp:5037 in 8727492. [](commit_id = 8727492, deletion_comment = False)

ianwjhalliday · 2016-08-18T01:21:03Z

if (pnode->sxFnc.IsAsync())

Note for later, this may not be necessary with how we fix default parameter expression evaluation. The try catch could probably go in the C++ code that does the initial next() call on the generator.

Refers to: lib/Runtime/ByteCode/ByteCodeEmitter.cpp:2897 in 8727492. [](commit_id = 8727492, deletion_comment = False)

ianwjhalliday · 2016-08-18T01:37:15Z

lib/Runtime/Library/JavascriptGeneratorFunction.cpp

+
+        Var* argsHeapCopy = RecyclerNewArray(scriptContext->GetRecycler(), Var, stackArgs.Info.Count);
+        js_memcpy_s(argsHeapCopy, sizeof(Var) * stackArgs.Info.Count, stackArgs.Values, sizeof(Var) * stackArgs.Info.Count);
+        Arguments heapArgs(callInfo, argsHeapCopy);


Are we getting two heap copies of the arguments since we're doing another heap allocation and copy above in CreateGenerator? We only need one heap allocated copy of the arguments.

Please check.

Removed it.

ianwjhalliday · 2016-08-18T01:48:27Z

I found a few cases where there was old code specific to async functions that no longer applied, in bytecode generator. I did not do a look through our code base though. Please do a quick look through search results for IsAsync to see if there are any other special cases that should be removed.

ianwjhalliday · 2016-08-18T01:50:02Z

nit: the title of this PR is incorrect. We were desugaring before, now we are implementing it natively (taking advantage of heavy reuse of JavascriptGeneratorFunction).

ianwjhalliday · 2016-08-18T22:17:48Z

lib/Parser/ptree.h

@@ -338,6 +338,7 @@ struct PnFnc
    bool IsDeclaration() const { return HasFlags(kFunctionDeclaration); }
    bool IsGeneratedDefault() const { return HasFlags(kFunctionIsGeneratedDefault); }
    bool IsGenerator() const { return HasFlags(kFunctionIsGenerator); }
+    bool IsCoroutine() const { return HasFlags(kFunctionIsGenerator) || HasFlags(kFunctionIsAsync); }


HasFlags accepts multiple flags, so you could do HasFlags(kFunctionIsGenerator | kFunctionIsAsync) instead. Doesn't matter though.

Or at least HasFlags should support multiple flags, I'm not sure if that's ever been used and thus tested

akroshg · 2016-08-19T18:10:45Z

lib/Runtime/Library/JavascriptLibrary.cpp

@@ -5869,6 +5934,15 @@ namespace Js
        return RecyclerNewEnumClass(this->GetRecycler(), EnumFunctionClass, JavascriptGeneratorFunction, type, scriptFunction);
    }

+    JavascriptAsyncFunction* JavascriptLibrary::CreateAsyncFunction(JavascriptMethod entryPoint, GeneratorVirtualScriptFunction* scriptFunction)
+    {
+        Assert(scriptContext->GetConfig()->IsES6GeneratorsEnabled());


Is this the right assert here?

Yes, we need the generator functionality all available for the async functions. But right now generators are enabled by default still I will just keep it there for now.

akroshg · 2016-08-19T18:12:02Z

Overall looks good to me. Are you planning to add debugger tests as well?

aneeshdk · 2016-08-19T18:57:35Z

I will add debugger test cases in a separate change set

dilijev · 2016-08-19T23:45:33Z

@dotnet-bot test Windows x64_debug please

…sues This change removes the function wrapper implementation of async functions. Now the async functions will behave similar to generator functions. The actual function body will be in the sciptFunction field. Symbols now need not be forced to slots. asyncspawn opcode and parse node are no more needed.In the backend async functions are also added to the generator JIT flag. await is desugared into yield.

… the symbol capturing issues Merge pull request #1456 from aneeshdk:AsyncFuncAsGenerator This change removes the function wrapper implementation of async functions. Now the async functions will behave similar to generator functions. The actual function body will be in the sciptFunction field. Symbols now need not be forced to slots. asyncspawn opcode and parse node are no more needed.In the backend async functions are also added to the generator JIT flag. await is desugared into yield.

aneeshdk force-pushed the AsyncFuncAsGenerator branch from 73fbef9 to 8727492 Compare August 17, 2016 17:22

msftclas added the cla-not-required label Aug 17, 2016

ianwjhalliday reviewed Aug 17, 2016
View reviewed changes

ianwjhalliday reviewed Aug 18, 2016
View reviewed changes

aneeshdk force-pushed the AsyncFuncAsGenerator branch 2 times, most recently from 3ed055a to 5e26878 Compare August 18, 2016 21:58

aneeshdk changed the title ~~Updating async function implementation by desugaring to generators~~ Updating async function implementation to fix the symbol capturing issues Aug 18, 2016

ianwjhalliday reviewed Aug 18, 2016
View reviewed changes

aneeshdk force-pushed the AsyncFuncAsGenerator branch from 5e26878 to 9e3ce48 Compare August 18, 2016 23:59

akroshg reviewed Aug 19, 2016
View reviewed changes

aneeshdk force-pushed the AsyncFuncAsGenerator branch 2 times, most recently from 00795c5 to 2d71a1a Compare August 19, 2016 18:56

aneeshdk force-pushed the AsyncFuncAsGenerator branch from 90f2d97 to a99327b Compare August 22, 2016 20:50

chakrabot merged commit a99327b into chakra-core:master Aug 22, 2016

snyk-bot mentioned this pull request Sep 16, 2021

[Snyk] Security upgrade yargs from 4.8.1 to 15.0.1 saurabharch/ChakraCore#5

Open

Updating async function implementation to fix the symbol capturing issues #1456

Updating async function implementation to fix the symbol capturing issues #1456

Uh oh!

Conversation

aneeshdk commented Aug 17, 2016

Uh oh!

aneeshdk commented Aug 17, 2016

Uh oh!

ianwjhalliday Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

aneeshdk Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

ianwjhalliday commented Aug 18, 2016

Uh oh!

ianwjhalliday commented Aug 18, 2016

Uh oh!

ianwjhalliday commented Aug 18, 2016

Uh oh!

ianwjhalliday Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

aneeshdk Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

ianwjhalliday commented Aug 18, 2016

Uh oh!

ianwjhalliday commented Aug 18, 2016

Uh oh!

ianwjhalliday Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

akroshg Aug 19, 2016

Choose a reason for hiding this comment

Uh oh!

aneeshdk Aug 19, 2016

Choose a reason for hiding this comment

Uh oh!

akroshg commented Aug 19, 2016

Uh oh!

aneeshdk commented Aug 19, 2016

Uh oh!

dilijev commented Aug 19, 2016

Uh oh!

Uh oh!