Generator misc #6533
Generator misc #6533
Conversation
|
@ppenzin this is a collection of trivial changes that had previously been ride-alongs in PRs related to enabling the JIT for generators, still not ready to do that but would like to get these small fry landed. The test failure is just the usual windows 7 disk space error, will hold on merging until we're up and running post-migration. |
rhuanjl
force-pushed
the
generatorMisc
branch
from
2a98b9a to
df3026f
Compare
|
I just updated this to latest following the TLA PR being merged - tests pass offline, windows 7 failure is the usual disk space issue. |
|
Updated to latest to see if checks still pass - still awaiting review then will squash before merging. |
|
@ppenzin Would be great if you could take a look at this - the 6 commits in it are each quite small and largely self-contained. |
There was a problem hiding this comment.
I've reached 'Cache AsyncGenerator Continuations', but haven't really reviewed that one yet. It seems to be easier going commit by commit, but github annoyingly disables "viewed" button in that mode.
I am going to keep plugging along, should not take very long to review everything.
| @@ -37,7 +37,6 @@ class LinearScanMD : public LinearScanMDShared | |||
| void GenerateBailOut(IR::Instr * instr, | |||
| __in_ecount(registerSaveSymsCount) StackSym ** registerSaveSyms, | |||
| uint registerSaveSymsCount); | |||
| IR::Instr* GenerateBailInForGeneratorYield(IR::Instr* resumeLabelInstr, BailOutInfo* bailOutInfo); | |||
There was a problem hiding this comment.
It was a draft Jit implementation for resuming a generator function. Whilst jitting generators is still not enabled the current implementation no longer uses this code.
There was a problem hiding this comment.
Can some of it be reused in the future, when we enable that?
There was a problem hiding this comment.
The Generator Jit implementation whilst still not enabled was re-written almost completely by @nhat-nguyen and further edited pretty significantly by @zenparsing - the current version works except when it hits a bailout in the wrong place which is a bug we need to solve before enabling it.
This bit of code is left over from the first implementation prior to the work the two of them did - it's not compatible with what they've done and is just left as noise in the source tree.
| @@ -49,7 +49,8 @@ Var JavascriptAsyncGeneratorFunction::EntryAsyncGeneratorFunctionImplementation( | |||
|
|
|||
| auto* asyncGeneratorFn = VarTo<JavascriptAsyncGeneratorFunction>(function); | |||
| auto* library = scriptContext->GetLibrary(); | |||
| auto* prototype = library->CreateAsyncGeneratorConstructorPrototypeObject(); | |||
| auto* prototype = VarTo<DynamicObject>(JavascriptOperators::GetPropertyNoCache( | |||
There was a problem hiding this comment.
This is more of a question about this commit, 5400255, why is the prototype creation unnecessary?
There was a problem hiding this comment.
Looking back over this noticed a further edit to make - will push an update, process prior to this change when calling a generator function gf was:
- Create a new generator prototype
- Create a generator object
objusing that new prototype - Call a different method to read the prototype off of
gfand overwriteobj's prototype with it
The change you've reviewed was replacing step 1 there instead reading the prototype off of gf BUT that meant we were doing:
- Read prototype off of
gf - Create a generator object
objusing that prototype - Call a different method to read the prototype off of
gf(again) and overwriteobj's prototype with it (even though it's the same)
My further change will delete step 3
There was a problem hiding this comment.
I've pushed the update - I squashed it into the same commit so you can check just Remove unnecessary prototype creation to see the change you'd already reviewed and the addition.
| @@ -37,7 +37,6 @@ class LinearScanMD : public LinearScanMDShared | |||
| void GenerateBailOut(IR::Instr * instr, | |||
| __in_ecount(registerSaveSymsCount) StackSym ** registerSaveSyms, | |||
| uint registerSaveSymsCount); | |||
| IR::Instr* GenerateBailInForGeneratorYield(IR::Instr* resumeLabelInstr, BailOutInfo* bailOutInfo); | |||
There was a problem hiding this comment.
Can some of it be reused in the future, when we enable that?
|
I know this hasn't been a common practice in CC, but having longer commit description would help with reviews, as reviewers won't ask as many questions. |
I thought this set was all quite simple but I will try and be more detailed in future. |
This PR brings in a few minor enhancements that were stuck in PRs with other things that could not be landed.
There's a functional change to fix the error for incorrect
thisvalues used in AsyncGenerator.prototype.next a few tests are added for this.The other changes are clean up, cacheing a few objects to prevent freeing/reallocating, removal of unnecessary memory allocations and some completely dead code.
EDIT: added a hack to make the TTD code handle Await Objects - tests were failing because it did not, I think the fact it passed before (i.e. without these changes) was due to the tests missing a code path that cacheing of await objects now causes us to hit.