| Summary: | [JSC] Invalidate old scope operations using global lexical binding epoch | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Yusuke Suzuki
2019-01-18 18:23:13 PST
Created attachment 359573 [details]
Patch
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.
Created attachment 359575 [details]
Patch
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.
Created attachment 359576 [details]
Patch
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.
Created attachment 359578 [details]
Patch
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.
Created attachment 359579 [details]
Patch
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.
Created attachment 359580 [details]
Patch
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.
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 Created attachment 359625 [details]
Patch
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.
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. 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). Created attachment 359657 [details]
Patch
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.
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 r=me 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. Created attachment 359661 [details]
Patch for landing
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.
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 Created attachment 359662 [details]
Patch for landing
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.
Comment on attachment 359662 [details] Patch for landing Clearing flags on attachment: 359662 Committed r240220: <https://trac.webkit.org/changeset/240220> Committed r240248: <https://trac.webkit.org/changeset/240248> 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. Reopening to attach new patch. Created attachment 359720 [details]
Patch
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. 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.
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
Comment on attachment 359720 [details] Patch Clearing flags on attachment: 359720 Committed r240254: <https://trac.webkit.org/changeset/240254> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 359846 [details]
Follow-up
(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. Committed r240329: <https://trac.webkit.org/changeset/240329> |