794679 - IonMonkey: Cache for GetPcScript()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 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.

Comment 1

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

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.

Comment 2

[Sean Stangl [:sstangl]]

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.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: 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)

Comment 4

[Nicolas B. Pierron [:nbp]]

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.

Comment 5

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f625a0dc1052

Thanks for the quick review!

Comment 6

[Ed Morley [:emorley]]

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