815161 - IonMonkey: cache result of prototype fetch for createThis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

See testcase in bug #813773:

// IM: 1.0xxs // With patch in #813773
// JM: 0.778s

We are still fraction slower with IM VS JM. This is because we don't cache the result of prototype fetch for the scripted createThis. When using MGetPropertyCache instead of MCallGetProperty I get the following time:

// IM: 0.792s // With patch in #813773

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: WIP: Use caching get property

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Use caching get property

./jit_test.py --ion $JS/js
PASSED ALL

I don't know all internals of GetPropertyCache, but this works for me and passes all tests. So if there is a corner-case I overlooked, let me know.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 685180 [details] [diff] [review]
Use caching get property

I think Jan would be a better reviewer for the idempotent caching.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 685180 [details] [diff] [review]
Use caching get property

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

::: js/src/ion/IonBuilder.cpp
@@ +3512,5 @@
>      // Get callee.prototype.
>      // This instruction MUST be idempotent: since it does not correspond to an
>      // explicit operation in the bytecode, we cannot use resumeAfter(). But
>      // calling GetProperty can trigger a GC, and thus invalidation.
> +    MGetPropertyCache *getProto = MGetPropertyCache::New(callee, cx->names().classPrototype);

Idempotent caches can trigger invalidation. To avoid a ton of these, we set a flag on the JSScript to stop using idempotent caches after we invalidate the first time. I don't think this cache can trigger invalidation, but it seems best to be safe and do something like:

if (!invalidatedIdempotentCache()) {
   ... MGetPropertyCache code you added ...
} else {
   ... old MCallGetProperty code ...
}

@@ +3513,5 @@
>      // This instruction MUST be idempotent: since it does not correspond to an
>      // explicit operation in the bytecode, we cannot use resumeAfter(). But
>      // calling GetProperty can trigger a GC, and thus invalidation.
> +    MGetPropertyCache *getProto = MGetPropertyCache::New(callee, cx->names().classPrototype);
> +    getProto->setResultType(MIRType_Value);

Nit: MIRType_Value is the default for GetPropertyCache so no need to set it.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

If the GetPropertyCache attaches an IC, that can GC and cause invalidation too.

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Use caching get property

- added fallback to MCallGetProperty
- added more comments :D
- renamed markUneffectful to setIdempotent, because they mean the same ... And else we had to do getPropCache->setIdempotent();, but callGetProp->markUneffectful(); however they do the same. I prefer it this way. Feel free to veto against.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 687058 [details] [diff] [review]
Use caching get property

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

Looks good.

Comment 8

[Hannes Verschore [:h4writer]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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