1155946 - Weird inconsistency between the getprop and getname case in baseline ICs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider these two testcases, run in a worker:

  for (var i = 0; i < 100000; ++i) performance;

and:

  var s = self; for (var i = 0; i < 100000; ++i) s.performance;

The latter is a lot slower than the former: over 2 orders of magnitude.

What happens in Ion is that it tries to do the getPropTryCommonGetter thing, and finds a baseline stub in the former case but not the latter.  So the latter ends up trying to use an Ion IC, which it can never generate because the prop lives on some proto of the global and the IC code does LookupPropertyPure (in CanAttachNativeGetProp) and LookupPropertyPure fails out if it hits a resolve hook and hasn't found the property (possibly on that object itself) yet.  And globals _always_ have a resolve hook, pretty much....

OK, so we should consider making the IC stuff better in Ion, but why the difference in baseline stubs?

That comes from the fact that TryAttachNativeGetAccessorPropStub does an EffectlesslyLookupProperty, which ends up using LookupPropertyPure, which as discussed above fails out.

On the other hand, TryAttachGlobalNameAccessorStub just does a manual walk up the proto chain while it's got native objects, doing a current->lookup() at each step.

So I guess the question is: is TryAttachGlobalNameAccessorStub unsound or is TryAttachNativeGetAccessorPropStub being too pessimistic in its assumptions?

And an even more important question is: how do we make this consistently fast?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I thought about this a bit, and I think the problem is in the getprop stub, kinda.  At least for resolve hooks on globals.

Specifically, I believe we can make the following assumption about resolve hooks on our globals: they are time-invariant.  That is, if you do a lookup and the resolve hook does NOT resolve it, then it won't in the future either.

If we can make this assumption, then we should allow ICs even in the face of globals' resolve hooks, because ICs do shape checks.  So when we're generating the IC we do a lookup, and there are three possibilities:

1)  We've already done a real lookup of this property on this object.  Then the resolve hook has already done its thing and there is no problem.

2)  We have not done a real lookup, but when we do end up doing it the resolve hook does not resolve this property name.  Clearly no problem.

3)  We have not done a real lookup, and when we do it the resolve hook will resolve this property name.  This will reshape the relevant object and if it was a delegate its proto chain, so the IC stub we generated (which found the property on some prototype of the thing with the resolve hook) will no longer match.  This should also not be a problem.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Script for testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

Comment 5

[Boris Zbarsky [:bzbarsky]]

We really do need to start growing some sort of set of DOM microbenchmarks that we run in automation.  We've been talking about this a decade... :(

Comment 6

[Jan de Mooij [:jandem]]

What's the global resolve hook in the browser, ResolveGlobal in BindingUtils.cpp or nsGlobalWindow::DoResolve?

Instead of special-casing more resolve hooks in LookupPropertyPure, it might be nice to have a new class hook - mayResolve(jsid id) - and have LookupPropertyPure call that instead of the current whitelisting. We could add some strong asserts: if the resolve hook sets *resolved to true, MOZ_ASSERT(clasp->mayResolve(id)).

Does this make sense? Are there other DOM classes with resolve hooks that could benefit from this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> What's the global resolve hook in the browser

Both.  The BindingUtils.cpp ResolveGlobal is called by both Window and Worker global resolve hooks (and in fact _is_ the resolve hook for Worker).  The Window resolve hook also calls nsGlobalWindow::DoResolve.

> Does this make sense?

What would mayResolve do, exactly?  Just return true or false depending on whether the resolve hook would resolve the property?  That _does_ require duplicating a bunch of code in the Window case, which won't be terribly fun...  And it would involve calling the various "is foo enabled?" functions, which have no guarantees about not having side-effects, unfortunately.  Though I suppose we could have mayResolve be a bit pessimistic and just return true in those cases without calling those functions...

Of course this also means that we'd need a way to ask the JS engine whether JS_ResolveStandardClass would resolve a given id.

> Are there other DOM classes with resolve hooks that could benefit from this?

The only DOM classes with resolve hooks are globals, HTMLEmbedElement, HTMLAppletElement, HTMLObjectElement, and Navigator.  The three elements have resolve hooks that do little things like mutate the proto chain, so there's no sane way to optimize those (in the mayResolve world they'd just return true for every id, I expect).  The Navigator resolve hook is pretty similar to the window one in terms of how it works; I guess the mayResolve setup could help there.  It's just that we haven't found anything that really depends on the performance of navigator.something.

I'm happy to try doing this if you're willing to hook up the JSAPI bits.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: WIP

This patch adds the mayResolve hook to JSFunction, StringObject and the shell's global object class.

I can build the browser with this applied. Can you see if this works with your changes?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8594987 [details] [diff] [review]
WIP

This works great.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add mayResolve methods to DOM classes with resolve hooks

Peter, I just used an infallible string allocation for the id, but if you prefer I can do a fallible one, returning true if the allocation fails.  Please let me know.

Comment 11

[Jan de Mooij [:jandem]]

There's an annoying case: JS_ResolveStandardClass does global->getOrCreateObjectPrototype to make sure the global has a proto. In other words, the global's proto can be lazily initialized by the resolve hook :(

I tried to move this getOrCreateObjectPrototype call to GlobalObject::new_ and that works in the shell but it breaks XPConnect's sandbox: we call sandbox_addProperty under getOrCreateObjectPrototype *before* we set the compartment's private and sandbox_addProperty crashes.

The simpler fix is to add a |JSObject* maybeObj| argument to the mayResolve hook and JS_MayResolveStandardClass, and have the hook check that. IonBuilder does not always know the JSObject*, so there we can call mayResolve with maybeObj == nullptr.

bz, it should be trivial to rebase your patch on top of this. I'll do some Try-servering.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add a mayResolve class hook

This patch adds a |mayResolve| hook, to replace the resolve hook whitelisting we currently do in LookupPropertyPure and IonBuilder's ObjectHasExtraOwnProperty.

The idea is that classes with a resolve hook can (optionally) have this extra, non-effectful hook to help JIT optimizations and other places that do pure lookups.

CallResolveOp asserts mayResolve returns true if we resolved a property, that's a pretty strong correctness check.

I converted JSFunction, StringObject and the shell's global to this new model.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 merged on top of jandem's changes to pass a maybeObject

Comment 14

[Boris Zbarsky [:bzbarsky]]

There's now an awfy test for this: http://arewefastyet.com/#machine=17&view=single&suite=assorteddom&subtest=misc-worker-getprop-performance-getter

Comment 15

[Boris Zbarsky [:bzbarsky]]

Also, I did verify that with the hazard annotations in part 2 the hazard analysis stuff on try passes.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed771ecfecd

Comment 17

[Jan de Mooij [:jandem]]

Pushed part 1, it's pretty big so I want to avoid bitrot.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ed771ecfecd

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 8595409 [details] [diff] [review]
Part 2 merged on top of jandem's changes to pass a maybeObject

Review of attachment 8595409 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +2226,5 @@
> +/* static */
> +bool
> +Navigator::MayResolve(jsid aId)
> +{
> +  // Note: Keep this in sync with DoResolve.

I think we should add a similar comment to DoResolve.

::: dom/base/Navigator.h
@@ +282,5 @@
>                   JS::Handle<jsid> aId,
>                   JS::MutableHandle<JSPropertyDescriptor> aDesc);
> +  // NOTE: This function does not fail and may not have any
> +  // side-effects.  The return value is whether DoResolve might end up
> +  // resolving the given id.  If in doubt, return true.

I'd put most of this comment in or next to the implementations (describing the return value here still makes sense).

::: dom/base/nsGlobalWindow.cpp
@@ +4273,5 @@
> +/* static */
> +bool
> +nsGlobalWindow::MayResolve(jsid aId)
> +{
> +  // Note: Keep this in sync with DoResolve.

Same here.

::: dom/base/nsGlobalWindow.h
@@ +502,5 @@
>                   JS::Handle<jsid> aId,
>                   JS::MutableHandle<JSPropertyDescriptor> aDesc);
> +  // NOTE: This function does not fail and may not have any side-effects.  The
> +  // return value is whether DoResolve might end up resolving the given id.  If
> +  // in doubt, return true.

Same here.

::: dom/base/nsObjectLoadingContent.h
@@ +172,5 @@
>                     JS::Handle<jsid> aId,
>                     JS::MutableHandle<JSPropertyDescriptor> aDesc);
> +    // NOTE: This function does not fail and may not have any side-effects.  The
> +    // return value is whether DoResolve might end up resolving the given id.
> +    // If in doubt, return true.

Same here.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc969d371858

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc969d371858