Moving cachedScope field out of ScriptFunction #5132
Conversation
| { | ||
| pnodeFnc->SetHasCachedScope(); | ||
| top->byteCodeFunction->GetFunctionInfo()->SetHasCachedScope(); | ||
| /*top->originalAttributes = (Js::FunctionInfo::Attributes)(top->originalAttributes | Js::FunctionInfo::Attributes::CachedScope);*/ |
There was a problem hiding this comment.
Can the commented line be deleted?
lib/Parser/ptree.h
Outdated
| @@ -433,7 +433,7 @@ enum FncFlags : uint | |||
| kFunctionIsClassConstructor = 1 << 18, // function is a class constructor | |||
| kFunctionIsBaseClassConstructor = 1 << 19, // function is a base class constructor | |||
| kFunctionIsClassMember = 1 << 20, // function is a class member | |||
| // Free = 1 << 21, | |||
| kFunctionHasCachedScope = 1 << 21, // Function has cached scopes | |||
There was a problem hiding this comment.
Can you point out the benefit of having this recorded on the parse node, since the byte code generator is already recording the attribute on the FuncInfo?
There was a problem hiding this comment.
Used for creating the attributes in GetFunctionInfoAttributes
|
With this change, are we able to figure out, for all deferred functions that will have NewScFunc byte code emitted for them, which ones will require scope objects when we execute them? Given that that's what we need to do, I was expecting a bigger change. |
lib/Runtime/Base/FunctionInfo.h
Outdated
| @@ -78,6 +79,8 @@ namespace Js | |||
| static bool IsCoroutine(Attributes attributes) { return ((attributes & (Async | Generator)) != 0); } | |||
| bool IsCoroutine() const { return IsCoroutine(this->attributes); } | |||
| bool HasComputedName() const { return (this->attributes & Attributes::ComputedName) != 0; } | |||
| bool HasCachedScope() const { return (this->attributes & Attributes::CachedScope) != 0; } | |||
There was a problem hiding this comment.
A naming question: is this attribute recording the fact that a function has a cached scope or that it needs a scope object and thus may have a cached scope? Seems like it's the latter. Does that change the way we want to describe this attribute?
lib/Runtime/Library/ScriptFunction.h
Outdated
| virtual void SetCachedScope(ActivationObjectEx *obj) override { cachedScopeObj = obj; } | ||
|
|
||
| #if ENABLE_TTD | ||
| virtual void MarkVisitKindSpecificPtrs(TTD::SnapshotExtractor* extractor) |
There was a problem hiding this comment.
any reason why this is not in the .cpp file?
| @@ -494,6 +494,7 @@ class ParseNodeFnc : public ParseNode | |||
| bool canBeDeferred; | |||
| bool isBodyAndParamScopeMerged; // Indicates whether the param scope and the body scope of the function can be merged together or not. | |||
| // We cannot merge both scopes together if there is any closure capture or eval is present in the param scope. | |||
| bool needsScopeObject; | |||
There was a problem hiding this comment.
nit: should we move the all bools together (from line 464) for better packing?
aneeshdk
force-pushed
the
MoveCachedScopeOutOfScriptFunc
branch
2 times, most recently
from
3362834 to
165a284
Compare
|
@pleath yes it looks like this covers all cases for scope objects including with statements. |
|
There are many calls to Scope::SetIsObject in the byte code generator that are not touched by your change. All that logic now has to be applied to top-level deferred functions as well as non-deferred ones. |
cachedscope is not applicable to most of the normal functions. This change moves it out of the ScriptFunction to to a separate derived class which is instantiated if we decide to have cachedscope for that particular function.
aneeshdk
force-pushed
the
MoveCachedScopeOutOfScriptFunc
branch
from
165a284 to
c8d82c4
Compare
cachedscope is not applicable to most of the normal functions. This change moves it out of the ScriptFunction to to a separate derived class which is instantiated if we decide to have cachedscope for that particular function.