Closed
Bug 794679
Opened 12 years ago
Closed 12 years ago
IonMonkey: Cache for GetPcScript()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: sstangl, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
5.97 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
||
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 4•12 years ago
|
||
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 | ||
Comment 5•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/f625a0dc1052
Thanks for the quick review!
Reporter | ||
Updated•12 years ago
|
Summary: IonMonkey: Have caches forward script, pc to GetPcScript() when known. → IonMonkey: Cache for GetPcScript()
Comment 6•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•