Optimize away throwing exception objects no one will ever see

NEW
Unassigned

Status

()

5 years ago
a year ago

People

(Reporter: bzbarsky, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.

Comment 2

5 years ago
Created attachment 8349795 [details] [diff] [review]
wip

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)
(Reporter)

Comment 3

5 years ago
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.
(Reporter)

Comment 4

5 years ago
Created attachment 8349835 [details] [diff] [review]
hackypatch on top that I used to test the DOM side of this
(Reporter)

Updated

5 years ago
Attachment #8349795 - Flags: feedback?(bzbarsky) → feedback+

Comment 5

5 years ago
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 :)

Comment 6

5 years ago
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...

Comment 7

5 years ago
Created attachment 8349850 [details] [diff] [review]
wip 2

Fix script->main()/script->code() bug.
Attachment #8349795 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
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.

Comment 10

5 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 1218972
Luke, do you have any plans to get back to this?
Flags: needinfo?(luke)
Unfortunately not.
Flags: needinfo?(luke)
You need to log in before you can comment on or make changes to this bug.