Closed Bug 849388 Opened 9 years ago Closed 8 years ago

IonMonkey: Allow Ion's InlineFrameIterator to be used in NoGC contexts

Categories

(Core :: JavaScript Engine, defect)

22 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: djvj, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently, InlineFrameIterator fails rooting analysis when used in NoGC contexts because it holds a rooted Function and Script pointer.

Baseline is running into this because it uses InlineFrameIterator from inside ion::Invalidate.
Attached patch Patch. (obsolete) — Splinter Review
Overview of changes:

There are some changes here to mainline GC code - mostly minor.  Unrooted<T> needs an implicit constructor from FakeRooted<T>.

I templatized ion::InlineFrameIterator and renamed it to ion::InlineFrameIteratorMaybeGC<AllowGC>.  InlineFrameIterator is now a typedef to |InlineFrameIteratorMaybeGC<CanGC>|.

Templatizing this class requires moving a bunch of code from a couple of .cpp files into IonFrames-inl.h.

Outside of that, there were some uses of Rooted<*> inside ion's CodeGenerator that have been changed to Unrooted.  And a couple of functions in ExecutionModeInlines.h changed from accepting Handle<*> to accepting Unrooted<*>.

Debug builds w/ root analysis pass jit-tests, both with and without --ion-parallel-compile.  Try push was just made (for Linux64 debug only).  Waiting for results on that.
Attachment #722939 - Flags: review?(terrence)
Attachment #722939 - Flags: review?(dvander)
Oh.  This patch was made against the ionmonkey branch, but the vast majority of changes are applicable to trunk, and I'll probably land it on trunk instead of baseline.

The only baseline specific change in this patch is in 'Ion.cpp', and a one-liner (changing InlineFrameIterator to InlineFrameIteratorNoGC).
Comment on attachment 722939 [details] [diff] [review]
Patch.

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

::: js/src/gc/Root.h
@@ +419,5 @@
>      inline Unrooted(const Rooted<S> &root,
>                      typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value, int>::Type dummy = 0);
> +    template <typename S>
> +    inline Unrooted(const FakeRooted<S> &root,
> +                    typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value, int>::Type dummy = 0);

Unrooted is gone as of yesterday, so you shouldn't need this hunk anymore.

@@ +897,5 @@
> +{
> +    JS_ASSERT(ptr_ != UninitializedTag());
> +    JS::EnterAssertNoGCScope();
> +}
> +#endif /* DEBUG */

Likewise.

::: js/src/ion/ExecutionModeInlines.h
@@ +39,5 @@
>      }
>      JS_NOT_REACHED("No such execution mode");
>  }
>  
> +static inline bool CanIonCompile(UnrootedScript script, ExecutionMode cmode)

Unrooted is gone now, so just JSScript *, likewise for JSFunction * and any others in this patch.

::: js/src/ion/IonFrameIterator.h
@@ +288,5 @@
>  };
>  
>  // Reads frame information in callstack order (that is, innermost frame to
>  // outermost frame).
> +template <AllowGC allowGC=CanGC>

Usually I see spaces around the = for default parameters. Is no spaces the style in IonMonkey? In either case, you probably don't need the default because of the typedefs.

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ -368,5 @@
>  // registers are evicted by the register allocator.
>  bool
>  CodeGeneratorShared::callVM(const VMFunction &fun, LInstruction *ins, const Register *dynStack)
>  {
> -    AssertCanGC();

Should already be gone on inbound.
Attachment #722939 - Flags: review?(terrence) → review+
Comment on attachment 722939 [details] [diff] [review]
Patch.

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

Use forward declaration of templates instead of moving all these functions into the headers.
The compilation time of each file of the JS engine is already awful, there is no reason to make that worse.

::: js/src/ion/IonFrames.cpp
@@ -1460,5 @@
>      }
>  }
> -
> -void
> -InlineFrameIterator::dump() const

Do not move these functions to the header file.
Attachment #722939 - Flags: feedback-
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 722939 [details] [diff] [review]
> Patch.
> 
> Review of attachment 722939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Use forward declaration of templates instead of moving all these functions
> into the headers.
> The compilation time of each file of the JS engine is already awful, there
> is no reason to make that worse.
> 
> ::: js/src/ion/IonFrames.cpp
> @@ -1460,5 @@
> >      }
> >  }
> > -
> > -void
> > -InlineFrameIterator::dump() const
> 
> Do not move these functions to the header file.

Ok, I will try to revert those changes where I can.
Comment on attachment 722939 [details] [diff] [review]
Patch.

Echoing what Nicolas said, would be good to not move these all to the header.
Attachment #722939 - Flags: review?(dvander)
Attached patch Try 2.Splinter Review
Some of the smaller methods had to move to the IonFrameIterator-inl.h because of the compiler complaining about "instantiation after use" for particular specializations.

However, the big methods are able to stay where they are.
Attachment #722939 - Attachment is obsolete: true
Attachment #723777 - Flags: review?(nicolas.b.pierron)
Attachment #723777 - Flags: review?(dvander)
Comment on attachment 723777 [details] [diff] [review]
Try 2.

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

::: js/src/ion/ExecutionModeInlines.h
@@ +49,5 @@
>      JS_NOT_REACHED("No such execution mode");
>      return false;
>  }
>  
> +static inline bool CanIonCompile(JSContext *cx, RawFunction fun, ExecutionMode cmode)

nit: Remove this cx.

::: js/src/ion/IonFrameIterator-inl.h
@@ +51,5 @@
>  
> +template <AllowGC allowGC>
> +inline
> +InlineFrameIteratorMaybeGC<allowGC>::InlineFrameIteratorMaybeGC(
> +                                JSContext *cx, const IonFrameIterator *iter)

nit: update argument indentation

@@ +76,5 @@
> +        findNextFrame();
> +    }
> +}
> +
> +template <AllowGC allowGC> template <class Op>

nit: Add a new line between the 2 templates.

::: js/src/ion/IonFrameIterator.h
@@ +336,5 @@
>      bool isFunctionFrame() const;
>      bool isConstructing() const;
> +    inline JSObject *scopeChain() const;
> +    inline JSObject *thisObject() const;
> +    inline InlineFrameIteratorMaybeGC &operator++();

Should we really inline these functions?  Is that worth it in terms of performances?

@@ +349,3 @@
>  };
> + 
> +typedef InlineFrameIteratorMaybeGC<CanGC> InlineFrameIterator;

nit: remove trailing whitespace above this line.

::: js/src/ion/IonFrames.cpp
@@ +1373,5 @@
>  }
> +template void InlineFrameIteratorMaybeGC<NoGC>::dump() const;
> +template void InlineFrameIteratorMaybeGC<CanGC>::dump() const;
> +} // namespace ion
> +} // namespace js

nit: I think you can encapsulate more functions into these 2 namespaces instead of adding them only for each function.
Attachment #723777 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Should we really inline these functions?  Is that worth it in terms of
> performances?

I was more concerned with the compiler complaining if we ended up having multiple callers for those methods and there being multiple non-inlined instantiations of the methods.

Are we guaranteed that won't happen?

> nit: I think you can encapsulate more functions into these 2 namespaces
> instead of adding them only for each function.

You're right, I can wrap the whole file with it.  Also allows us to remove a bunch
of extraneous 'js::ion' and 'ion::' namespace qualifiers there.
Comment on attachment 723777 [details] [diff] [review]
Try 2.

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

Sorry for the delay, I missed the review request.
Attachment #723777 - Flags: review?(dvander) → review+
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9b303c9161

Try push used to verify this patch:
https://tbpl.mozilla.org/?tree=Try&rev=91b58a249733

Leaving open because there is some minor changes to land on ionmonkey (baseline compiler) tree for this bug.
Whiteboard: [leave open]
Depends on: 853777
Forgot to close this.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.