Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give NewScFunc the correct nested function index when we transfrom GetCachedFunc to NewScFunc in the backend #3424

Merged
merged 3 commits into from
Aug 7, 2017

Conversation

rajatd
Copy link
Contributor

@rajatd rajatd commented Jul 25, 2017

As part of stack args optimization, we remove all references to scope object from the IR. One of the instructions referencing the scope object (cached scope object in this case) is GetCachedFunc, which loads the nested function object from a particulat index from the cached scope. In the deadstore phase, we change it to NewScFunc and create a new function object instead of using the cached one.

Slot values in the cached scope and the function body's nested function array may correspond to different nested functions. In this situation, we were incorrectly setting the index of the nested function on NewScFunc by reusing the index from GetCachedFunc.

@rajatd rajatd changed the title Give NewScFunc the correct nested function index when it we transfrom GetCachedFunc to NewScFunc in the backend Give NewScFunc the correct nested function index when we transfrom GetCachedFunc to NewScFunc in the backend Jul 25, 2017
@rajatd
Copy link
Contributor Author

rajatd commented Jul 28, 2017

/cc @Microsoft/chakra-developers , can someone review this PR?

@dilijev
Copy link
Contributor

dilijev commented Aug 2, 2017

Please update GUID in lib\Runtime\ByteCode\ByteCodeCacheReleaseFileVersion.h and rebase on top of the latest release/1.6 to avoid the merge conflict on bytecode.

@@ -63,6 +63,8 @@ namespace Js
const int magicEndOfAsmJsModuleInfo = *(int*)"]asmmodinfo";
const int magicStartOfPropIdsOfFormals = *(int*)"propIdOfFormals[";
const int magicEndOfPropIdsOfFormals = *(int*)"]propIdOfFormals";
const int magicStartOfSlotIdToNestedIndexArray = *(int*)"slotIdToNestedIndexArray[";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, this magic part is not what it's supposed to be :) your usage accidentally works- the dbgscope ones dont- the magic reinterprets the first 4 chars of a string as a uint32 so you really want only 4 char strings- in the case of magicStartOfDebuggerScopes and magicStartOfDebuggerScopeProperties, the first 4 chars are the same- really all of these longer than 4 chars should be fixed to be unique 4 char strings (3 + [)

@digitalinfinity
Copy link
Contributor

    PrependStruct<SerializedFieldList>(builder, _u("Serialized Field List"), &definedFields);

Instead of using a byte for whether your field exists, can you add a bit in this struct?


Refers to: lib/Runtime/ByteCode/ByteCodeSerializer.cpp:2228 in db2510c. [](commit_id = db2510c, deletion_comment = False)

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, changes in ByteCodeSerializer and FunctionBody LGTM- minor comments


instr->ReplaceSrc1(intConstOpnd);
uint nestedFuncIndex = instr->m_func->GetJITFunctionBody()->GetNestedFuncIndexForSlotIdInCachedScope(intConstOpnd->AsIntConstOpnd()->AsUint32());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well Free intConstOpnd...

@LouisLaf
Copy link
Collaborator

LouisLaf commented Aug 3, 2017

:shipit:

@chakrabot chakrabot merged commit a5bcd44 into chakra-core:release/1.6 Aug 7, 2017
chakrabot pushed a commit that referenced this pull request Aug 7, 2017
…function index when we transfrom GetCachedFunc to NewScFunc in the backend

Merge pull request #3424 from rajatd:slotIdToNestedIndex

As part of stack args optimization, we remove all references to scope object from the IR. One of the instructions referencing the scope object (cached scope object in this case) is GetCachedFunc, which loads the nested function object from a particulat index from the cached scope. In the deadstore phase, we change it to NewScFunc and create a new function object instead of using the cached one.

Slot values in the cached scope and the function body's nested function array may correspond to different nested functions. In this situation, we were incorrectly setting the index of the nested function on NewScFunc by reusing the index from GetCachedFunc.
chakrabot pushed a commit that referenced this pull request Aug 8, 2017
…ct nested function index when we transfrom GetCachedFunc to NewScFunc in the backend

Merge pull request #3424 from rajatd:slotIdToNestedIndex

As part of stack args optimization, we remove all references to scope object from the IR. One of the instructions referencing the scope object (cached scope object in this case) is GetCachedFunc, which loads the nested function object from a particulat index from the cached scope. In the deadstore phase, we change it to NewScFunc and create a new function object instead of using the cached one.

Slot values in the cached scope and the function body's nested function array may correspond to different nested functions. In this situation, we were incorrectly setting the index of the nested function on NewScFunc by reusing the index from GetCachedFunc.
chakrabot pushed a commit that referenced this pull request Aug 8, 2017
…unc the correct nested function index when we transfrom GetCachedFunc to NewScFunc in the backend

Merge pull request #3424 from rajatd:slotIdToNestedIndex

As part of stack args optimization, we remove all references to scope object from the IR. One of the instructions referencing the scope object (cached scope object in this case) is GetCachedFunc, which loads the nested function object from a particulat index from the cached scope. In the deadstore phase, we change it to NewScFunc and create a new function object instead of using the cached one.

Slot values in the cached scope and the function body's nested function array may correspond to different nested functions. In this situation, we were incorrectly setting the index of the nested function on NewScFunc by reusing the index from GetCachedFunc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants