[webkit-reviews] review canceled: [Bug 193603] [JSC] Invalidate old scope operations using global lexical binding epoch : [Attachment 359625] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 20 16:08:51 PST 2019


Yusuke Suzuki <ysuzuki at apple.com> has canceled Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 193603: [JSC] Invalidate old scope operations using global lexical binding
epoch
https://bugs.webkit.org/show_bug.cgi?id=193603

Attachment 359625: Patch

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




--- Comment #18 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 359625
  --> https://bugs.webkit.org/attachment.cgi?id=359625
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(UniquedStringImp
l* 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).


More information about the webkit-reviews mailing list