Bug 193603 – [JSC] Invalidate old scope operations using global lexical binding epoch

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Invalidate old scope operations using global lexical binding epoch

[Yusuke Suzuki]

Created attachment 359573 [details]
Patch

[EWS Watchlist]

Attachment 359573 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 359575 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/47380869>

[EWS Watchlist]

Attachment 359575 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 359576 [details]
Patch

[EWS Watchlist]

Attachment 359576 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 359578 [details]
Patch

[EWS Watchlist]

Attachment 359578 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 359579 [details]
Patch

[EWS Watchlist]

Attachment 359579 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 359580 [details]
Patch

[EWS Watchlist]

Attachment 359580 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 359580 [details]
Patch

Attachment 359580 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10813221

New failing tests:
stress/scope-operation-cache-global-property-before-deleting.js.ftl-eager-no-cjit-b3o1
stress/let-lexical-binding-shadow-existing-global-property-ftl.js.ftl-eager-no-cjit-b3o1
stress/const-lexical-binding-shadow-existing-global-property-ftl.js.dfg-eager-no-cjit-validate
stress/const-lexical-binding-shadow-existing-global-property-ftl.js.no-llint
stress/scope-operation-cache-global-property-before-deleting.js.no-llint
stress/let-lexical-binding-shadow-existing-global-property-ftl.js.ftl-eager-no-cjit
stress/scope-operation-cache-global-property-before-deleting.js.ftl-eager-no-cjit
stress/const-lexical-binding-shadow-existing-global-property-ftl.js.ftl-eager-no-cjit-b3o1
stress/let-lexical-binding-shadow-existing-global-property-ftl.js.dfg-eager-no-cjit-validate
stress/let-lexical-binding-shadow-existing-global-property-ftl.js.no-llint
stress/scope-operation-cache-global-property-before-deleting.js.dfg-eager-no-cjit-validate
apiTests

[Yusuke Suzuki]

Created attachment 359625 [details]
Patch

[EWS Watchlist]

Attachment 359625 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 359625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359625&action=review

> Source/JavaScriptCore/dfg/DFGDesiredGlobalProperties.cpp:52
> +        property.globalObject()->ensureReferencedPropertyWatchpointSet(uid).fireAll(vm, "Lexical binding shadows the existing global properties");

"shadows the existing global properties" => "shadows an existing global property"

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1065
> +    // but this is not done yet since we do not execute this op again. Just emitting ForceOSRExit to update the metadata when it reaches to this code.

"but this is not done yet since we do not execute this op again" => something like... "but we still have stale metadata here since we have not yet executed this bytecode operation since the invalidation"

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:815
>          JSScope** constantScopeSlot = metadata.m_constantScope.slot();
>          emitVarInjectionCheck(needsVarInjectionChecks(resolveType));
>          loadPtr(constantScopeSlot, regT0);

I don't think this code makes sense any more. I think it just works if you call emitCode(resolveType) since that will do the epoch check.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1072
> +    case GlobalPropertyWithVarInjectionChecks: // Global Lexical Binding Epoch is changed. Update op_resolve_scope.

Don't think this comment adds much.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1889
> +WatchpointSet* JSGlobalObject::getReferencedPropertyWatchpointSetConcurrently(UniquedStringImpl* uid)

Why call this "concurrently" if that's the only option? Seems like we just call it with the old name.

> Source/JavaScriptCore/runtime/Options.h:511
> +    v(unsigned, thresholdForGlobalLexicalBindingEpoch, UINT_MAX, Normal, "Threshold for global lexical binding epoch. If the epoch reaches to this value, CodeBlock metadata for scope operations will be revised globally. It needs to be greater than 1.") \

Let's add a RELEASE_ASSERT in Options.cpp or just bump it to UINT_MAX if this is zero. I think I prefer the latter solution

> Source/JavaScriptCore/runtime/ProgramExecutable.cpp:210
> +            // So that we do not create WatchpointSet here. DFG will create if necessary on the main thread.

And it will only create it if the global lexical environment binding doesn't exist, which is why this code works.

^ I'd add something like that to this comment.

[Yusuke Suzuki]

Comment on attachment 359625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359625&action=review

>> Source/JavaScriptCore/dfg/DFGDesiredGlobalProperties.cpp:52
>> +        property.globalObject()->ensureReferencedPropertyWatchpointSet(uid).fireAll(vm, "Lexical binding shadows the existing global properties");
> 
> "shadows the existing global properties" => "shadows an existing global property"

Fixed.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1065
>> +    // but this is not done yet since we do not execute this op again. Just emitting ForceOSRExit to update the metadata when it reaches to this code.
> 
> "but this is not done yet since we do not execute this op again" => something like... "but we still have stale metadata here since we have not yet executed this bytecode operation since the invalidation"

Sounds nice. Fixed.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:815
>>          loadPtr(constantScopeSlot, regT0);
> 
> I don't think this code makes sense any more. I think it just works if you call emitCode(resolveType) since that will do the epoch check.

OK, use emitCode to construct this code path. We still need to switch the code based on metadata.resolveType since op_resolve_scope+GlobalProperty would be updated to op_resolve_scope+GlobalLexicalVar.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1072
>> +    case GlobalPropertyWithVarInjectionChecks: // Global Lexical Binding Epoch is changed. Update op_resolve_scope.
> 
> Don't think this comment adds much.

Dropped.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1889
>> +WatchpointSet* JSGlobalObject::getReferencedPropertyWatchpointSetConcurrently(UniquedStringImpl* uid)
> 
> Why call this "concurrently" if that's the only option? Seems like we just call it with the old name.

OK, fixed.

>> Source/JavaScriptCore/runtime/Options.h:511
>> +    v(unsigned, thresholdForGlobalLexicalBindingEpoch, UINT_MAX, Normal, "Threshold for global lexical binding epoch. If the epoch reaches to this value, CodeBlock metadata for scope operations will be revised globally. It needs to be greater than 1.") \
> 
> Let's add a RELEASE_ASSERT in Options.cpp or just bump it to UINT_MAX if this is zero. I think I prefer the latter solution

OK, added correctOptions() function which corrects option values if it is out of range.

>> Source/JavaScriptCore/runtime/ProgramExecutable.cpp:210
>> +            // So that we do not create WatchpointSet here. DFG will create if necessary on the main thread.
> 
> And it will only create it if the global lexical environment binding doesn't exist, which is why this code works.
> 
> ^ I'd add something like that to this comment.

Added it. DFG will create not-invalidated watchpoint set only if the global lexical environment binding doesn't exist. (to avoid compile-and-fail loop, DFG also creates invalidated watchpoint set if it attempted to watch the GlobalProperty that is shadowed).

[Yusuke Suzuki]

Created attachment 359657 [details]
Patch

[EWS Watchlist]

Attachment 359657 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Options.cpp:382:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 359657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359657&action=review

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:818
>          JSScope** constantScopeSlot = metadata.m_constantScope.slot();

might as well do the same thing here

[Saam Barati]

r=me

[Yusuke Suzuki]

Comment on attachment 359657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359657&action=review

>> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:818
>>          JSScope** constantScopeSlot = metadata.m_constantScope.slot();
> 
> might as well do the same thing here

Oops, right. Fixed.

[Yusuke Suzuki]

Created attachment 359661 [details]
Patch for landing

[EWS Watchlist]

Attachment 359661 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Options.cpp:382:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 359661 [details]
Patch for landing

Rejecting attachment 359661 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 359661, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/10823482

[Yusuke Suzuki]

Created attachment 359662 [details]
Patch for landing

[EWS Watchlist]

Attachment 359662 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Options.cpp:382:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 359662 [details]
Patch for landing

Clearing flags on attachment: 359662

Committed r240220: <https://trac.webkit.org/changeset/240220>

[Yusuke Suzuki]

Committed r240248: <https://trac.webkit.org/changeset/240248>

[Yusuke Suzuki]

Comment on attachment 359657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359657&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2163
> +        loadi OpResolveScope::Metadata::m_globalLexicalBindingEpoch[globalObject], scratch

This was wrong. It should be `t5` for metadata table here.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 359720 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 359720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359720&action=review

Annotated the changes from the previous one.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:629
> +                    metadata.m_globalLexicalBindingEpoch = m_globalObject->globalLexicalBindingEpoch();

Set the current epoch, since JSScope::abstractResolve already ensured this is correct.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2105
> +        loadi OpResolveScope::Metadata::m_globalLexicalBindingEpoch[t5], scratch

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2163
> +        loadi OpResolveScope::Metadata::m_globalLexicalBindingEpoch[t5], scratch

This `t5` was previously `globalObject`, but it was wrong.

[EWS Watchlist]

Attachment 359720 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.h:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Options.cpp:382:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

                                           ToT                     Patched
SunSpider:
   3d-cube                            4.6991+-0.0880     ?      4.7298+-0.0750        ?
   3d-morph                           5.3163+-0.0393            5.2699+-0.0391
   3d-raytrace                        4.9223+-0.0675     ?      4.9307+-0.0567        ?
   access-binary-trees                2.0875+-0.0325     ?      2.1027+-0.0332        ?
   access-fannkuch                    5.1875+-0.1310            5.0740+-0.1209          might be 1.0224x faster
   access-nbody                       2.8640+-0.0419     ?      2.8751+-0.0358        ?
   access-nsieve                      2.6054+-0.1617            2.5496+-0.1412          might be 1.0219x faster
   bitops-3bit-bits-in-byte           1.4706+-0.0284     ?      1.4765+-0.0285        ?
   bitops-bits-in-byte                2.7107+-0.0381            2.6668+-0.0329          might be 1.0165x faster
   bitops-bitwise-and                 2.3497+-0.0372     ?      2.3927+-0.0334        ? might be 1.0183x slower
   bitops-nsieve-bits                 3.6324+-0.0260            3.6200+-0.0297
   controlflow-recursive              2.3468+-0.0371     ?      2.3478+-0.0337        ?
   crypto-aes                         4.1436+-0.0492            4.1068+-0.0515
   crypto-md5                         2.7557+-0.0348     ?      2.7578+-0.0387        ?
   crypto-sha1                        3.0644+-0.1375            2.9451+-0.0418          might be 1.0405x faster
   date-format-tofte                  7.0749+-0.0944            7.0373+-0.0595
   date-format-xparb                  5.7545+-0.0401     ?      5.8003+-0.0873        ?
   math-cordic                        3.0190+-0.0345            3.0072+-0.0342
   math-partial-sums                  4.2980+-0.0398     ?      4.3634+-0.0377        ? might be 1.0152x slower
   math-spectral-norm                 2.0313+-0.0342     ?      2.0389+-0.0312        ?
   regexp-dna                         7.5687+-0.0900     ?      7.6086+-0.1278        ?
   string-base64                      4.2967+-0.0419     ?      4.3243+-0.0527        ?
   string-fasta                       6.2167+-0.0621     ?      6.2472+-0.1422        ?
   string-tagcloud                    8.1186+-0.0805            8.1171+-0.0849
   string-unpack-code                16.7391+-0.2687           16.6323+-0.2328
   string-validate-input              4.9366+-0.0780     ?      4.9824+-0.0758        ?

   <arithmetic>                       4.6235+-0.0195            4.6155+-0.0139          might be 1.0017x faster

                                           ToT                     Patched
Octane:
   encrypt                           0.14941+-0.00059    ?     0.14975+-0.00064       ?
   decrypt                           2.95386+-0.01838    ?     2.96075+-0.01959       ?
   deltablue                x2       0.12588+-0.00362    ?     0.12637+-0.00306       ?
   earley                            0.21826+-0.00137          0.21666+-0.00097
   boyer                             3.58181+-0.02607          3.56802+-0.02056
   navier-stokes            x2       4.22469+-0.06589          4.18124+-0.02478         might be 1.0104x faster
   raytrace                 x2       0.73508+-0.00516          0.73258+-0.00245
   richards                 x2       0.08183+-0.00103          0.08158+-0.00079
   splay                    x2       0.22986+-0.00233          0.22856+-0.00127
   regexp                   x2      12.87470+-0.11864         12.73747+-0.10437         might be 1.0108x faster
   pdfjs                    x2      26.95156+-0.24909    ?    27.16141+-0.23212       ?
   mandreel                 x2      33.07604+-0.13692    ?    33.43279+-0.34674       ? might be 1.0108x slower
   gbemu                    x2      23.06035+-0.16657    ?    23.27870+-0.26941       ?
   closure                           0.40390+-0.00103    ?     0.40686+-0.00300       ?
   jquery                            5.28154+-0.07343          5.24535+-0.02954
   box2d                    x2       6.67657+-0.03759          6.65511+-0.03653
   zlib                     x2     269.18828+-5.31932        266.83823+-1.74256
   typescript               x2     486.49224+-4.01904    ?   492.18654+-9.78357       ? might be 1.0117x slower
   splay-latency                     1.78478+-0.02078    ?     1.79133+-0.02318       ?

   <geometric>                       3.97328+-0.01269          3.97296+-0.01141         might be 1.0001x faster

                                           ToT                     Patched
Geomean of preferred means:
   <scaled-result>                   13.5534+-0.0363           13.5414+-0.0329          might be 1.0009x faster

[WebKit Commit Bot]

Comment on attachment 359720 [details]
Patch

Clearing flags on attachment: 359720

Committed r240254: <https://trac.webkit.org/changeset/240254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 359846 [details]
Follow-up

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #40)
> Created attachment 359846 [details]
> Follow-up

Discussed with Saam offline, and we agree that this is OK.

[Yusuke Suzuki]

Committed r240329: <https://trac.webkit.org/changeset/240329>