Closed Bug 798638 Opened 8 years ago Closed 8 years ago

GC: exact rooting for TryNoteIter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v0 (obsolete) — Splinter Review
TryNoteIter stores a JSScript* and lives on the stack.  This switches it to a RootedScript.
Attachment #668664 - Flags: review?(luke)
Comment on attachment 668664 [details] [diff] [review]
v0

Review of attachment 668664 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ +3977,5 @@
>      gc::MaybeVerifyBarriers(cx, true);
>      return interpReturnOK ? Interpret_Ok : Interpret_Error;
> +
> +  exit_error_to_len:
> +    DO_NEXT_OP(len);

Why is this necessary?  Each weird goto makes reasoning about Interpret that much more confusing, so it would be really good to avoid this.
Comment on attachment 668664 [details] [diff] [review]
v0

Review of attachment 668664 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.h
@@ +269,5 @@
>  
>  class TryNoteIter
>  {
>      const FrameRegs &regs;
> +    RootedScript script;

Is it worth commenting that this is safe because objects of this class are always stack-allocated?
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Is it worth commenting that this is safe because objects of this class are
> always stack-allocated?

Good idea!  Done.
Assignee: general → terrence
Attachment #668664 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #668664 - Flags: review?(luke)
Attachment #669723 - Flags: review?(luke)
Comment on attachment 669723 [details] [diff] [review]
v1: Longer, with fewer inexplicable branches.

Review of attachment 669723 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ +820,5 @@
> +      case JSTRAP_RETURN:
> +        cx->clearPendingException();
> +        regs.fp()->setReturnValue(rval);
> +      case JSTRAP_THROW:
> +        cx->setPendingException(rval);

These cases seem to be missing break statements.

@@ +3938,3 @@
>          }
>  
> +        switch(UnwindForCatchableException(cx, regs, script)) {

I'm not sure that this factoring of CallThrowHook/UnwindForCatchableException really makes anything simpler: it doesn't simplify control flow or add any real abstractions, it just kinda pulls code apart.

IIRC, removing indirect goto would solve these problems; can we just do that (as a prerequesite bug)?
Attachment #669723 - Flags: review?(luke)
Depends on: 799777
Attached patch v2Splinter Review
Now that the interpreter loop simplification has stuck, this is thoroughly trivial to implement.
Attachment #669723 - Attachment is obsolete: true
Attachment #679254 - Flags: review?(luke)
Attachment #679254 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/c0893be94207
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.