Open Bug 951924 Opened 10 years ago Updated 2 years ago

Optimize away throwing exception objects no one will ever see

Categories

(Core :: JavaScript Engine, defect)

defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Luke suggested this as an idea for the slowness of throwing an exception that the caller catches and does not examine.

Basically, have a way to quickly check (even in ioncode!) whether we're at a pc that's inside a try block whose catch block never uses the exception.  And presumably also that we're not in debug mode.  And if those conditions hold, we can just throw undefined instead of the actual exception object, since no one can tell the difference.
A slightly further reaching, trickier to implement alternative is to change the internals of try a bit.  Rather than statically inferring the state of 'excepts will be caught and discarded', explicitly store this state in a global-ish location (e.g. JSContext).  Upon entering a new try block, store the old value of this state on the stack, so it can be restored later.  This way, we can get the same optimization any time an exception will be ignored, no matter where it is actually caught.
Attached patch wip (obsolete) — Splinter Review
bz: perhaps you can take this patch for a spin?

Marty's idea is a good one and would provide better high-end performance (currentScript() isn't super-cheap) if we were realllly pounding on this.  OTOH, it'd be significantly more invasive than this patch so I'd be interested to see where this patch leaves us on the relevant jQuery benchmark.
Attachment #8349795 - Flags: feedback?(bzbarsky)
Well, that patch microbenchmarks pretty well, except in a toplevel script where it seems to have no effect.  But inside a function, it speeds up my microbenchmarks by about 10x.  Profiling the result, about 10% of the time is spent doing WillThrowExceptionBeIgnored, which is pretty good.  60% of the time at that point is js::jit::HandleException, and a large chunk of that is jit::BaselineScript::nativeCodeForPC.

On the more "real-life" testcase in attachment 811992 [details], without this patch I see times like so (in ms):

  1904 1825 1788 1826 1799

with the patch I see times like so:

  1788 1785 1716 1801 1713

So it seems to help some, for sure.
Attachment #8349795 - Flags: feedback?(bzbarsky) → feedback+
Hmm, I guess this isn't really the hottest spot, then for attachment 911992 (unless it's hitting the global script case?).

Jan,Kannan: think we can optimize jit::HandleException?  I'm thinking maybe a per-BaselineScript single-entry cache for nativeCodeForPC :)
Also, from IRC: could you verify that WillThrownExceptionBeIgnored is actually returning 'true' in the global testcase?  (Maybe my bytecode "analysis" is failing on some different bytecode that is emitted at toplevel.)  Assuming there isn't some other hotspot, I'd expect that just saving the malloc of the stack and DOM exception object would be good enough...
Attached patch wip 2Splinter Review
Fix script->main()/script->code() bug.
Attachment #8349795 - Attachment is obsolete: true
Attachment 811992 [details] is not hitting the global script case; it's just doing a bunch of work in addition to the throwing querySelectorAll call.

I thought we had another case that was hitting the exception-throwing more, but I can't find the relevant bug right now...

Also, note that the numbers in comment 3 are both with my existing patches for bug 932837.
I think Marty's suggestion would also involve pushing the "exceptions are handled" stack when calling into C++, or else extra (possibly error-prone) work annotating JSNatives that may catch and handle exceptions.

>+    if (script->compartment()->debugMode())
>+        return false;

I wonder if this is good enough. There may be other compartments on the portion of the stack that will be unwound.
Good question.  At the moment, the predicate is quite conservative and it returns 'false' if it doesn't find a try block in the innermost script.  Thus, the debug-mode of the innermost script is all that matters.

On a random note: if we decide to land this optimization, it would be pretty easy to see if the catch block's binding has any uses in the parser and then record this info in the try note so that WillThrownExceptionBeIgnored can simply test the bit instead of pattern-matching the bytecode.
OS: Mac OS X → All
Hardware: x86 → All
Blocks: dom-requests
Luke, do you have any plans to get back to this?
Flags: needinfo?(luke)
Unfortunately not.
Flags: needinfo?(luke)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: