908881 - Suppress GC when reporting errors

Status: RESOLVED DUPLICATE of bug 1031881

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: hazard_error_reporting-v0.diff

Currently, we have to exactly root on all error paths, including JS_CHECK_RECURSION. If we do not allow GC during error reporting, we should be able to eliminate a large number of roots for a performance and simplicity win.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 794864 [details] [diff] [review]
hazard_error_reporting-v0.diff

Seems good, but please wait a day to push this in case other people here have an opinion.

Comment 2

[Jeff Walden [:Waldo]]

I don't actually think we care about performance in error paths, to be honest.  And I think consistency with other code is preferable to seeming simplicity wins.  (At least, unless roots and stuff actively interfere with whatever's being done -- as in GC -- but I don't see any interference between error reporting and rooting.)

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> I don't actually think we care about performance in error paths, to be
> honest.  And I think consistency with other code is preferable to seeming
> simplicity wins.  (At least, unless roots and stuff actively interfere with
> whatever's being done -- as in GC -- but I don't see any interference
> between error reporting and rooting.)

I would think this isn't about the error paths. I think it's more about having a bunch of functions that can only GC when they take an error path, which forces callers to root everything across them unconditionally. So the performance at stake is the performance in the normal case, which is suffering (slightly) due to the possibility of the error case.

Somebody correct me if I'm wrong.

That said, I see the ReportError calls a debugger hook. Is that JSD-style only, or is it used for Debugger as well? I know people would like to be able to detect exceptions that aren't going to be caught, at the point where the exception was thrown. That could involve executing a whole lot of JS code, and I would not want GC to be suppressed for all of that time. But I'm not sure if this is where that would happen. I don't see anywhere in vm/Debugger.cpp that sets the debugErrorHook, but I don't know if I'm looking in the right place. needinfo? jorendorff.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess it does call into a lot of jsd stuff. And looking at the Firebug code, it seems like a lot can happen there. Maybe we can't do this :-(.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #3)
Yes, that is correct.

(In reply to Bill McCloskey (:billm) from comment #4)
> I guess it does call into a lot of jsd stuff. And looking at the Firebug
> code, it seems like a lot can happen there. Maybe we can't do this :-(.

Dang! I had forgotten about that. The suppression we make for ReportOutOfMemory's callback happens to work because jsd immediately returns in that case. It does mean, however, that we cannot trivially assert we are not in a suppression at the top of Interpret either. :-(

Comment 6

[Jason Orendorff [:jorendorff]]

Two observations:

1. This is an old JSD1 hook. I think we can kill it off once Firebug is ported. So come back to this.

2. Even without removing it, looks like we already don't call the hook if we're at the point of stack overflow. I think a small change here would ensure that JS_CHECK_RECURSION can't GC.

Comment 8

[Terrence Cole [:terrence]]

And debugErrorHook is now being removed in 1031881, making this moot.