If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Suppress GC when reporting errors

RESOLVED DUPLICATE of bug 1031881

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 1031881
4 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 794864 [details] [diff] [review]
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.
Attachment #794864 - Flags: review?(wmccloskey)
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.
Attachment #794864 - Flags: review?(wmccloskey) → review+
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.)
(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.
Flags: needinfo?(jorendorff)
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 :-(.
(Assignee)

Comment 5

4 years ago
(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. :-(
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.
Flags: needinfo?(jorendorff)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 907807
(Assignee)

Updated

4 years ago
Blocks: 795030
No longer blocks: 898606
(Assignee)

Comment 8

3 years ago
And debugErrorHook is now being removed in 1031881, making this moot.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1031881
You need to log in before you can comment on or make changes to this bug.