When we construct our GlobalProperty, GlobalLexicalVar, etc. mechanism, we assumed that lexical binding cannot shadow global object's properties. But it is wrong. Wc can shadow it if it's configurable attribute is true. Let's see the example. // script1.js function foo() { bar = 4; } function foo2() { return bar; } foo(); print(foo2()); // => 4 // script2.js const bar = 5; print(foo2()); // => 4, but it should be 5! Once the reference to "bar" is configured as GlobalProperty, then, it attempts to load the value from JSGlobalObject. However, `const bar = 5` can shadow it, and we should not use GlobalProperty no longer for that.
The super simple solution is leveraging the existing VarInjectionCheck mechanism. But it is not acceptable since almost GlobalProperty now become GlobalPropertyWithVarInjectionCheck. I think we can leverage the nature of GlobalProperty load. 1. GlobalProperty performs structure check before loading like IC. 2. Shadowing should be super rare. We can discard the existing mis-cache by perform structure transition for JSGlobalObject. And in the slow path, we can transform GlobalProperty to GlobalLexicalVar if necessary.
Discussed with Saam. We should start with the slow, but memory-efficient and non-tricky way. Once the problematic thing happens, we start iterating all the CodeBlocks and convert instructions related to GlobalProperty to GlobalLexicalVar. And we also fire the watchpoint for a name to discard DFG and FTL code. These codes will be recompiled with GlobalLexicalVar configuration, it does not cause the jettison again with the same watchpoint for the same name.
Created attachment 358776 [details] Patch
Created attachment 358777 [details] Patch
We need to have watchpoint part next.
Created attachment 358811 [details] WIP I believe it starts working
<rdar://problem/45546542>
Created attachment 358827 [details] WIP
Comment on attachment 358827 [details] WIP Attachment 358827 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10701874 New failing tests: stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager stress/const-lexical-binding-shadow-existing-global-property.js.dfg-eager stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1 stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1 stress/const-lexical-binding-shadow-existing-global-property.js.dfg-eager-no-cjit-validate stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager-no-cjit-validate stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-small-pool stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-validate-sampling-profiler stress/let-lexical-binding-shadow-existing-global-property.js.default stress/let-lexical-binding-shadow-existing-global-property-baseline.js.default stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-b3o1 stress/let-lexical-binding-shadow-existing-global-property.js.dfg-maximal-flush-validate-no-cjit stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-validate-phases stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-collect-continuously stress/let-lexical-binding-shadow-existing-global-property.js.no-ftl stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-put-stack-validate stress/let-lexical-binding-shadow-existing-global-property.js.no-llint stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-inline-validate apiTests
Comment on attachment 358827 [details] WIP Attachment 358827 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10702008 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html
Created attachment 358845 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 358855 [details] Patch
Comment on attachment 358855 [details] Patch Attachment 358855 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10704352 New failing tests: stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1 stress/const-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit-b3o1 stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-validate-sampling-profiler stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-small-pool stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager stress/let-lexical-binding-shadow-existing-global-property.js.dfg-eager-no-cjit-validate stress/let-lexical-binding-shadow-existing-global-property.js.default stress/let-lexical-binding-shadow-existing-global-property-baseline.js.default stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager-no-cjit stress/let-lexical-binding-shadow-existing-global-property.js.ftl-eager stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-b3o1 stress/let-lexical-binding-shadow-existing-global-property.js.dfg-maximal-flush-validate-no-cjit stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-validate-phases stress/let-lexical-binding-shadow-existing-global-property.js.no-cjit-collect-continuously stress/let-lexical-binding-shadow-existing-global-property.js.no-ftl stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-put-stack-validate stress/let-lexical-binding-shadow-existing-global-property.js.no-llint stress/let-lexical-binding-shadow-existing-global-property.js.ftl-no-cjit-no-inline-validate apiTests
Created attachment 358872 [details] Patch
Created attachment 358875 [details] Patch
Created attachment 358877 [details] Patch
Created attachment 358879 [details] Patch
I think this patch works well on 64bit environments. The remaining things are, 1. Adding more tests. With eval(), FTL, DFG etc. 2. Adding 32bit supports.
(In reply to Yusuke Suzuki from comment #18) > I think this patch works well on 64bit environments. > The remaining things are, > > 1. Adding more tests. With eval(), FTL, DFG etc. > 2. Adding 32bit supports. And adding op_profile_type support.
Created attachment 358912 [details] WIP OK, the patch becomes mature. It is discovered that op_profile_type does not need any care. After adding more tests, it becomes ready.
Created attachment 358927 [details] Patch
Created attachment 358929 [details] Patch
Comment on attachment 358929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358929&action=review r=me > Source/JavaScriptCore/ChangeLog:26 > + In JSC, we cache GlobalProperty resolve type and its associated information in op_resolve_type, op_get_from_scope, op_put_to_scope, and op_profile_type. This patch doesn't deal with profile_type. > Source/JavaScriptCore/ChangeLog:37 > + to GlobalLexicalVar (or Dynamic precisely). So, the subsequent LLInt code just works well. you should explain the Dynamic for "const" thing > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2698 > + metadata.localScopeDepth = op.depth; Is this actually correct? This seems wrong, since previously, if the depth was N, we should now be at N-1. However, I think this will make us stay at N. (I don't think we end up actually using this for global lexical variables, but it's weird.) > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2745 > + resolveType = needsVarInjectionChecks(originalResolveType) ? GlobalLexicalVarWithVarInjectionChecks : GlobalLexicalVar; Why is this needed? Shouldn't this be an assert? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6191 > + m_graph.globalProperties().addLazily(DesiredGlobalProperty(globalObject, identifierNumber)); I think we should also do this for get_from_scope and put_to_scope. I'm not sure if we break this rule today (I don't think so), but I don't think we should rely on resolve_scope always happening before put_to_scope/get_from_scope > Source/JavaScriptCore/dfg/DFGPlan.cpp:574 > +bool Plan::syncInMainThreadForValidation() I'm not a fan of this name. Maybe "isStillValid" is enough? Or, "isStillValidOnMainThread" or something similar. > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:771 > + // Here, we get constant value for GlobalProperty and GlobalPropertyWithVarInjectionChecks. > + // This is because UnresolvedProperty already has a resolveType check. If a resolveType is changed > + // to GlobalLexicalVar, we do not enter the code for GlobalProperty. Not sure this comment is really needed. > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1716 > + // otherwise lexical bindings can change the result of GlobalVar queries too. Might be worth one more sentence saying we won't be able to declare a global lexical variable w/ this same name b/c configurable=false
Comment on attachment 358929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358929&action=review >> Source/JavaScriptCore/ChangeLog:26 >> + In JSC, we cache GlobalProperty resolve type and its associated information in op_resolve_type, op_get_from_scope, op_put_to_scope, and op_profile_type. > > This patch doesn't deal with profile_type. Oops, this remained. Fixed. >> Source/JavaScriptCore/ChangeLog:37 >> + to GlobalLexicalVar (or Dynamic precisely). So, the subsequent LLInt code just works well. > > you should explain the Dynamic for "const" thing OK, added. "Dynamic" happens when we attempt to put a value into a const binding, and it should throw a type error. >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2698 >> + metadata.localScopeDepth = op.depth; > > Is this actually correct? This seems wrong, since previously, if the depth was N, we should now be at N-1. However, I think this will make us stay at N. (I don't think we end up actually using this for global lexical variables, but it's weird.) Yes Actually, any value is OK here (and that's why I just used the calculated depth information). We do not use it at all after that. So any value is OK here. Maybe, for the marker, I think 0 is better. Fixed. To calculate the correct depth information, we need one of the actual JSScopes which will be used with this CodeBlock to perform abstractResolve / abstractAccess. But holding this JSScope in this CodeBlock's member can keep unnecessary memory. That's why we immediately pass globalScope here instead, and that is the reason why we can't calculate the correct depth information here. >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2745 >> + resolveType = needsVarInjectionChecks(originalResolveType) ? GlobalLexicalVarWithVarInjectionChecks : GlobalLexicalVar; > > Why is this needed? Shouldn't this be an assert? We calculate the resolve type from the global scope. So even if we had a scope which uses eval, we cannot find it in the above abstractResolve. But we do not have the actual scope chain here. But if the eval scope existed, this information must be existed in the originalResolveType since the originalResolveType should be GlobalProperty or GlobalPropertyWithVarInjectionChecks. So, we can use this information to correct the result here. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6191 >> + m_graph.globalProperties().addLazily(DesiredGlobalProperty(globalObject, identifierNumber)); > > I think we should also do this for get_from_scope and put_to_scope. I'm not sure if we break this rule today (I don't think so), but I don't think we should rely on resolve_scope always happening before put_to_scope/get_from_scope Make sense. Fixed. >> Source/JavaScriptCore/dfg/DFGPlan.cpp:574 >> +bool Plan::syncInMainThreadForValidation() > > I'm not a fan of this name. Maybe "isStillValid" is enough? Or, "isStillValidOnMainThread" or something similar. OK, changed to `isStillValidOnMainThread()`. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:771 >> + // to GlobalLexicalVar, we do not enter the code for GlobalProperty. > > Not sure this comment is really needed. Dropped. >> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1716 >> + // otherwise lexical bindings can change the result of GlobalVar queries too. > > Might be worth one more sentence saying we won't be able to declare a global lexical variable w/ this same name b/c configurable=false Yeah, nice. Added.
Created attachment 358942 [details] Patch for landing
Comment on attachment 358929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358929&action=review >>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2698 >>> + metadata.localScopeDepth = op.depth; >> >> Is this actually correct? This seems wrong, since previously, if the depth was N, we should now be at N-1. However, I think this will make us stay at N. (I don't think we end up actually using this for global lexical variables, but it's weird.) > > Yes Actually, any value is OK here (and that's why I just used the calculated depth information). We do not use it at all after that. So any value is OK here. Maybe, for the marker, I think 0 is better. Fixed. > To calculate the correct depth information, we need one of the actual JSScopes which will be used with this CodeBlock to perform abstractResolve / abstractAccess. But holding this JSScope in this CodeBlock's member can keep unnecessary memory. > That's why we immediately pass globalScope here instead, and that is the reason why we can't calculate the correct depth information here. Can we do the same in byte code linking to be consistent? >>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2745 >>> + resolveType = needsVarInjectionChecks(originalResolveType) ? GlobalLexicalVarWithVarInjectionChecks : GlobalLexicalVar; >> >> Why is this needed? Shouldn't this be an assert? > > We calculate the resolve type from the global scope. So even if we had a scope which uses eval, we cannot find it in the above abstractResolve. But we do not have the actual scope chain here. > But if the eval scope existed, this information must be existed in the originalResolveType since the originalResolveType should be GlobalProperty or GlobalPropertyWithVarInjectionChecks. > So, we can use this information to correct the result here. Got it. Makes sense.
Comment on attachment 358929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358929&action=review >>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2698 >>>> + metadata.localScopeDepth = op.depth; >>> >>> Is this actually correct? This seems wrong, since previously, if the depth was N, we should now be at N-1. However, I think this will make us stay at N. (I don't think we end up actually using this for global lexical variables, but it's weird.) >> >> Yes Actually, any value is OK here (and that's why I just used the calculated depth information). We do not use it at all after that. So any value is OK here. Maybe, for the marker, I think 0 is better. Fixed. >> To calculate the correct depth information, we need one of the actual JSScopes which will be used with this CodeBlock to perform abstractResolve / abstractAccess. But holding this JSScope in this CodeBlock's member can keep unnecessary memory. >> That's why we immediately pass globalScope here instead, and that is the reason why we can't calculate the correct depth information here. > > Can we do the same in byte code linking to be consistent? We can set it in CodeBlock::finishCreation.
Note that, if it affects on performance in the measurement, we can add an additional complexity to avoid it, 1. have a counter, and bump it once we introduce a lexical binding shadowing an existing one 2. when the counter is overflow, we can iterate CodeBlock and rewrite them as we are doing now. 3. based on counter, in the slow path, we can rewrite itself 4. in DFG and FTL, we check the watchpoint, and if it is invalidated, we start converting it to GlobalLexicalVar or, we make it ForceOSRExit at the moment.
Committed r239879: <https://trac.webkit.org/changeset/239879>
Committed r239898: <https://trac.webkit.org/changeset/239898>