Bug 65290 – DFG speculative JIT does not implement load elimination for GetById

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Filip Pizlo]

The DFG JIT has most of the information that it needs for performing load elimination on GetById.  Currently it only does load elimination for GetByVal.  It should also do it for GetById, and should take care to handle cases where GetById or PutById might cause a side-effect that invalidates the elimination of the load.

[Filip Pizlo]

Created attachment 102194 [details]
the patch

I'd let this patch simmer before reviewing it.  I'm not expecting it to pass EWS in its current state.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1

Source/JavaScriptCore/dfg/DFGAliasTracker.h:179:  Missing spaces around /  [whitespace/operators] [3]
Source/JavaScriptCore/runtime/JSObject.h:111:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:283:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSObject.h:382:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSValue.h:212:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:453:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/Operations.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:926:  The parameter name "recovery" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 25 files


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

[Filip Pizlo]

Created attachment 102201 [details]
the patch (fix style)

This fixes style.  I've also rerun tests on the Mac.  I will mark as ? when the other bots are happy.

[Filip Pizlo]

Created attachment 102215 [details]
the patch (fix windows)

More fixes in this patch, but I'm still suspecting bot fails.

[Filip Pizlo]

Created attachment 102228 [details]
the patch (attempt to fix windows, again)

[Gavin Barraclough]

Comment on attachment 102228 [details]
the patch (attempt to fix windows, again)

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

I think we need to blow aliases to same id on different bases, too, unless we can prove they are not the same value.

> Source/JavaScriptCore/dfg/DFGAliasTracker.h:123
> +            if (possibleAlias.child1() == node.child1()

This seems to only shoot down candidates for alias if they have the same base in the graph, but not if they have different bases.  But different bases may refer to the same object.
e.g. this will correctly prevent aliasing here (the two reads of a.y may read different values due to the assignment of b):
function f1(a,b) { var x = a.y; a.y = b; return x + a.y; }
however I think this will fail on the following:
function f2(a,b,c) { var x = a.y; b.y = c; return x + a.y; }
This time we won't detect that the write to b.y aliases a.y, so the assignment won't clear the alias & if the function is called with the same the object as a & b, then the latter read of a.y will incorrectly alias the original value rather than c.

I think we need to shoot down aliases in all cases where the identifier matches?

> Source/JavaScriptCore/dfg/DFGJITCompiler.h:57
> +enum CompilerKind { Speculative, NonSpeculative, EitherCompiler };

JITCodeGenerator already has a member called m_isSpeculative, I don't think we need both this & the CompilerKind enum.
It is probably better to use your enum since it is clearer & more descriptive, so I'd replace the member bool with a CompilerKind.
But I think we do need this to be a member in some cases, so I think you can remove passing CompilerKind as an augment to function on JITCompiler in a number of cases, and just use the member instead.

The key thing here is that we should unify these; we shouldn't have two ways of doing this.

[Filip Pizlo]

(In reply to comment #6)
> (From update of attachment 102228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102228&action=review
> 
> I think we need to blow aliases to same id on different bases, too, unless we can prove they are not the same value.
> 
> > Source/JavaScriptCore/dfg/DFGAliasTracker.h:123
> > +            if (possibleAlias.child1() == node.child1()
> 
> This seems to only shoot down candidates for alias if they have the same base in the graph, but not if they have different bases.  But different bases may refer to the same object.
> e.g. this will correctly prevent aliasing here (the two reads of a.y may read different values due to the assignment of b):
> function f1(a,b) { var x = a.y; a.y = b; return x + a.y; }
> however I think this will fail on the following:
> function f2(a,b,c) { var x = a.y; b.y = c; return x + a.y; }
> This time we won't detect that the write to b.y aliases a.y, so the assignment won't clear the alias & if the function is called with the same the object as a & b, then the latter read of a.y will incorrectly alias the original value rather than c.
> 
> I think we need to shoot down aliases in all cases where the identifier matches?
> 
> > Source/JavaScriptCore/dfg/DFGJITCompiler.h:57
> > +enum CompilerKind { Speculative, NonSpeculative, EitherCompiler };
> 
> JITCodeGenerator already has a member called m_isSpeculative, I don't think we need both this & the CompilerKind enum.
> It is probably better to use your enum since it is clearer & more descriptive, so I'd replace the member bool with a CompilerKind.
> But I think we do need this to be a member in some cases, so I think you can remove passing CompilerKind as an augment to function on JITCompiler in a number of cases, and just use the member instead.
> 
> The key thing here is that we should unify these; we shouldn't have two ways of doing this.

Gosh, good catch on both counts!  My bad, I'll fix that.

[Gavin Barraclough]

Oliver, could you eyeball the runtime changes to JSObject etc in this patch?

Let me summarize.  We need new get/put methods that will not cause side effects (e.g. call getter/setters), that can be used from the speculative path and relied on not to cause arbitrary code execution.

Filip has added a putLimitedSideEffect methods which will only write to regular properties on instances of the JSFinalObject class.  This seems safe.

getNoSideEffect is a little more subtle.  We've prohibited API objects (since they can do anything!) via the newly added propertyAccessesMayCauseArbitrarySideEffects() check.  Otherwise, this patch will walk the proto chain calling fastGetOwnPropertySlot.  Once it has the property slot it calls a new getValueNoSideEffect(), which won't call getters on it.  Does this sound safe to you?  Do we need guard against any objects in WebCore triggering arbitrary code execution from within a getOwnPropertySlot?

[Oliver Hunt]

(In reply to comment #8)
> Oliver, could you eyeball the runtime changes to JSObject etc in this patch?
> 
> Let me summarize.  We need new get/put methods that will not cause side effects (e.g. call getter/setters), that can be used from the speculative path and relied on not to cause arbitrary code execution.
> 
> Filip has added a putLimitedSideEffect methods which will only write to regular properties on instances of the JSFinalObject class.  This seems safe.
> 
> getNoSideEffect is a little more subtle.  We've prohibited API objects (since they can do anything!) via the newly added propertyAccessesMayCauseArbitrarySideEffects() check.  Otherwise, this patch will walk the proto chain calling fastGetOwnPropertySlot.  Once it has the property slot it calls a new getValueNoSideEffect(), which won't call getters on it.  Does this sound safe to you?  Do we need guard against any objects in WebCore triggering arbitrary code execution from within a getOwnPropertySlot?

There are a reasonable number of objects in webcore that override getOwnPropertySlot, some of them have side effects on the _get_, most notable they may throw exceptions due to same origin violations.

I believe the side-effect free lookup should do an explicit check for the overridesGetOwnPropertySlot() flag.  propertyAccessesMayCauseArbitrarySideEffects seems very fragile as it is essentially a blacklist, and historically blacklist based filtering has been problematic for us.  We're better off defaulting to property access being unsafe, and then opting in to this optimisation on a case by case basis (this only applies to classes that set overridesGetOwnPropertySlot).

TLDR;  this code is currently unsafe, there are a number of objects in the DOM where getOwnPropertySlot has immediate side effects

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #8)
> > Oliver, could you eyeball the runtime changes to JSObject etc in this patch?
> > 
> > Let me summarize.  We need new get/put methods that will not cause side effects (e.g. call getter/setters), that can be used from the speculative path and relied on not to cause arbitrary code execution.
> > 
> > Filip has added a putLimitedSideEffect methods which will only write to regular properties on instances of the JSFinalObject class.  This seems safe.
> > 
> > getNoSideEffect is a little more subtle.  We've prohibited API objects (since they can do anything!) via the newly added propertyAccessesMayCauseArbitrarySideEffects() check.  Otherwise, this patch will walk the proto chain calling fastGetOwnPropertySlot.  Once it has the property slot it calls a new getValueNoSideEffect(), which won't call getters on it.  Does this sound safe to you?  Do we need guard against any objects in WebCore triggering arbitrary code execution from within a getOwnPropertySlot?
> 
> There are a reasonable number of objects in webcore that override getOwnPropertySlot, some of them have side effects on the _get_, most notable they may throw exceptions due to same origin violations.
> 
> I believe the side-effect free lookup should do an explicit check for the overridesGetOwnPropertySlot() flag.  propertyAccessesMayCauseArbitrarySideEffects seems very fragile as it is essentially a blacklist, and historically blacklist based filtering has been problematic for us.  We're better off defaulting to property access being unsafe, and then opting in to this optimisation on a case by case basis (this only applies to classes that set overridesGetOwnPropertySlot).
> 
> TLDR;  this code is currently unsafe, there are a number of objects in the DOM where getOwnPropertySlot has immediate side effects

Is this summary true even if exceptions are fine?  By "no side effect" we mean that getOwnPropertySlot() will not modify the heap in some arbitrary way prior to returning.  And by modifying the heap we specifically mean: modify properties that don't involve getters (custom or not).

[Oliver Hunt]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Oliver, could you eyeball the runtime changes to JSObject etc in this patch?
> > > 
> > > Let me summarize.  We need new get/put methods that will not cause side effects (e.g. call getter/setters), that can be used from the speculative path and relied on not to cause arbitrary code execution.
> > > 
> > > Filip has added a putLimitedSideEffect methods which will only write to regular properties on instances of the JSFinalObject class.  This seems safe.
> > > 
> > > getNoSideEffect is a little more subtle.  We've prohibited API objects (since they can do anything!) via the newly added propertyAccessesMayCauseArbitrarySideEffects() check.  Otherwise, this patch will walk the proto chain calling fastGetOwnPropertySlot.  Once it has the property slot it calls a new getValueNoSideEffect(), which won't call getters on it.  Does this sound safe to you?  Do we need guard against any objects in WebCore triggering arbitrary code execution from within a getOwnPropertySlot?
> > 
> > There are a reasonable number of objects in webcore that override getOwnPropertySlot, some of them have side effects on the _get_, most notable they may throw exceptions due to same origin violations.
> > 
> > I believe the side-effect free lookup should do an explicit check for the overridesGetOwnPropertySlot() flag.  propertyAccessesMayCauseArbitrarySideEffects seems very fragile as it is essentially a blacklist, and historically blacklist based filtering has been problematic for us.  We're better off defaulting to property access being unsafe, and then opting in to this optimisation on a case by case basis (this only applies to classes that set overridesGetOwnPropertySlot).
> > 
> > TLDR;  this code is currently unsafe, there are a number of objects in the DOM where getOwnPropertySlot has immediate side effects
> 
> Is this summary true even if exceptions are fine?  By "no side effect" we mean that getOwnPropertySlot() will not modify the heap in some arbitrary way prior to returning.  And by modifying the heap we specifically mean: modify properties that don't involve getters (custom or not).

My point was that you're relying on _all_ code conforming to an invariant we can't enforce statically at compile time.  If you were okay with exceptions being thrown you'd need to ensure you always cleared the exception slot.

I also just realised that getOwnPropertySlot on a JSFunction can modify the object among other things -- it should not be observable that the change has happened, but it does modify the object.

[Filip Pizlo]

> My point was that you're relying on _all_ code conforming to an invariant we can't enforce statically at compile time.  If you were okay with exceptions being thrown you'd need to ensure you always cleared the exception slot.

The latter part is easy, the former comes down to a question of risk/reward.  This patch wins us maybe 1%, and that's in an idealized fantasy world that involves the above-posted incorrect patch.  So my feeling is to table this and revisit later (or never), especially given that we have other, bigger things, to optimize.  Gavin, do you concur?

And yeah, I have thought about making this an opt-in rather than opt-out speculation, where for example we only rely on GetById on JSFinalObject being free of side effects.  This scares me as well because it would mean that any GetById on any non-JSFinalObject would currently knock us off the speculative path.  That's not good, given that this is only a 1% win *in the idealized case*, and the non-speculative path is known to be more than 1% slower than the speculative path.

> I also just realised that getOwnPropertySlot on a JSFunction can modify the object among other things -- it should not be observable that the change has happened, but it does modify the object.

Yeah, changes in general are OK - but changes that are observable with GetById are not OK.

[Sam Weinig]

I don't think we are going to do this.  We can revisit some other time.