IonMonkey: Cache for GetPcScript()

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 665190 [details] [diff] [review]
patch

Removes the expensive part of GetPcScript() by caching script and pc in the IonActivation when possible. Note that idempotent caches do not retain this information and therefore forward NULL.
Attachment #665190 - Flags: review?(dvander)
Comment on attachment 665190 [details] [diff] [review]
patch

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

We should check that it mitigates the issues that have been reported so far in, say, bug 794117, bug 791219, bug 790628, bug 789478. I'm not sure all the problem just occur from ICs, and if not, we should probably have a generic one-entry cache.

We might also have to mark IonCompartment::lastScript_, if for nothing else but moving GC - I'd check with the GC folks.

::: js/src/ion/IonCaches.cpp
@@ +316,4 @@
>      RootedScript script(cx);
>      jsbytecode *pc;
>      cache.getScriptedLocation(script.address(), &pc);
> +    AutoCachePcScript pcScriptCache(cx, script, pc);

Instead, I would make this something like:

AutoCachePcScript pcScriptCache(cx, cache);

and keep the RootedScript inside it.
Attachment #665190 - Flags: review?(dvander) → review+
(Reporter)

Comment 2

6 years ago
Timing from the above bugs as affected by this patch:

Bug 794117: 3.496 -> 3.472
Bug 791219: 1.035 -> 1.006
Bug 790628: 0.782 -> 0.764

So nothing is really affected by forwarding from caches and a more generic cache is needed.
(Reporter)

Comment 3

6 years ago
Created attachment 666746 [details] [diff] [review]
patch v2

New patch. Instead of trying to forward values, it just caches them at the exact point of GetPcScript(). The cache is very basic, self-contained, and doesn't require support from other parts of the engine, making it trivial to remove and with minimal overhead.

Perf from this patch:

Bug 794117: 3.554 -> 1.565 (1.494 --no-ion)
Bug 791219: 1.030 -> 0.534 (0.646 --no-ion)
Bug 790628: 0.780 -> 0.320 (0.401 --no-ion)
Attachment #665190 - Attachment is obsolete: true
Attachment #666746 - Flags: review?(nicolas.b.pierron)
Comment on attachment 666746 [details] [diff] [review]
patch v2

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

Looks good, I would prefer if the PcScriptCache could be more like a stand-alone cache structure and keep the logic inside it instead of moving parts of it's logic to GetPcScript.

::: js/src/ion/IonFrames.cpp
@@ +681,5 @@
> +    JSRuntime *rt = cx->runtime;
> +
> +    // Recover the return address.
> +    IonFrameIterator it(rt->ionTop);
> +    uint8_t *returnAddress = it.exitFrame()->returnAddress();

nit: it.returnAddress()

@@ +688,5 @@
> +
> +    // Attempt to look up address in cache.
> +    if (rt->ionPcScriptCache) {
> +        // Ensure that no GC has occurred, so all stored JSScripts are still valid.
> +        if (rt->ionPcScriptCache->gcNumber == rt->gcNumber) {

nit: Move the gcNumber logic to a PcScriptCache method, it would be better to don't see it here.

@@ +705,5 @@
> +    } else {
> +        // Initialize the cache. The allocation may safely fail and will not GC.
> +        rt->ionPcScriptCache = (PcScriptCache *)js_malloc(sizeof(struct PcScriptCache));
> +        if (rt->ionPcScriptCache)
> +            rt->ionPcScriptCache->clear(rt->gcNumber);

nit: I would prefer if you can hide the gcNumber stuff. can you only use JSRuntime pointer instead of a GC number.  And keep all the gcNumber logic inside PcScriptCache.

@@ +722,5 @@
> +    if (rt->ionPcScriptCache) {
> +        PcScriptCacheEntry *entry = &rt->ionPcScriptCache->entries[index];
> +        entry->returnAddress = returnAddress;
> +        entry->pc = ifi.pc();
> +        entry->script = ifi.script();

nit: rt->ionPcScriptCache->add(…) , hide the entry manipulation of the PcScriptCache.
Attachment #666746 - Flags: review?(nicolas.b.pierron) → review+
(Reporter)

Updated

6 years ago
Summary: IonMonkey: Have caches forward script, pc to GetPcScript() when known. → IonMonkey: Cache for GetPcScript()

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f625a0dc1052
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.