Closed Bug 815161 Opened 7 years ago Closed 7 years ago

IonMonkey: cache result of prototype fetch for createThis

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch WIP: Use caching get property (obsolete) — Splinter Review
Assignee: general → hv1989
Blocks: 768745
Attached patch Use caching get property (obsolete) — Splinter Review
./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.
Attachment #685173 - Attachment is obsolete: true
Attachment #685180 - Flags: review?(bhackett1024)
Comment on attachment 685180 [details] [diff] [review]
Use caching get property

I think Jan would be a better reviewer for the idempotent caching.
Attachment #685180 - Flags: review?(bhackett1024) → review?(jdemooij)
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.
Attachment #685180 - Flags: review?(jdemooij)
If the GetPropertyCache attaches an IC, that can GC and cause invalidation too.
- 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.
Attachment #685180 - Attachment is obsolete: true
Attachment #687058 - Flags: review?(jdemooij)
Comment on attachment 687058 [details] [diff] [review]
Use caching get property

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

Looks good.
Attachment #687058 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/e83393045b95
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.