Closed Bug 1333143 Opened 4 years ago Closed 4 years ago

Self-host Object.prototype.valueOf

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Self-hosting Object.prototype.valueOf makes it easier to use this function for bug 1333045. I had two change two tests because we can now enter JS code when calling this function.
Attachment #8829536 - Flags: review?(till)
Comment on attachment 8829536 [details] [diff] [review]
Self-host Object.prototype.valueOf

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

Looks good to me, modulo the comment below, and a question.

The question is whether this is perf-neutral, or whether valueOf should be inlined using JS_INLINABLE_FN. The ToObject will be inlined no matter what, but that still leaves valueOf potentially not being inlined if the inlining depth is exhausted. It also seems trivial to inline it.

::: js/src/wasm/AsmJS.cpp
@@ +7480,4 @@
>      // most apps have been built with newer Emscripten.
>      if (v.toObject().is<JSFunction>() &&
>          HasNoToPrimitiveMethodPure(&v.toObject(), cx) &&
> +        HasObjectValueOfMethodPure(&v.toObject(), cx) &&

What we really want to check is whether the object has the original _builtin_ function, not whether it's native, right? Instead of adding the valueOf one-off here, would it make sense to rename HasNativeMethodPure to HasBuiltinMethodPure and add an overload for self-hosted functions that takes the self-hosted name instead of the JSNative?
Attachment #8829536 - Flags: review?(till) → review+
(In reply to Till Schneidereit [till] from comment #1)
> Comment on attachment 8829536 [details] [diff] [review]
> Self-host Object.prototype.valueOf
> 
> Review of attachment 8829536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, modulo the comment below, and a question.
> 
> The question is whether this is perf-neutral, or whether valueOf should be
> inlined using JS_INLINABLE_FN. The ToObject will be inlined no matter what,
> but that still leaves valueOf potentially not being inlined if the inlining
> depth is exhausted. It also seems trivial to inline it.
> 
I mean before this was always a native call, so probably not the fastest thing. But you are right this is simple enough to inline that we should just do it.

> ::: js/src/wasm/AsmJS.cpp
> @@ +7480,4 @@
> >      // most apps have been built with newer Emscripten.
> >      if (v.toObject().is<JSFunction>() &&
> >          HasNoToPrimitiveMethodPure(&v.toObject(), cx) &&
> > +        HasObjectValueOfMethodPure(&v.toObject(), cx) &&
> 
> What we really want to check is whether the object has the original
> _builtin_ function, not whether it's native, right? Instead of adding the
> valueOf one-off here, would it make sense to rename HasNativeMethodPure to
> HasBuiltinMethodPure and add an overload for self-hosted functions that
> takes the self-hosted name instead of the JSNative?
I think what you are getting is that the name isn't perfect and I agree. The idea is to check for specific builtin method, which thus far has been native (built-in) methods. We just know that these methods have no side-effects and thus this hack should not be observable. My general idea of putting this inside AsmJS.cpp was the slim hope that we wouldn't need this anywhere else and could remove it at the same time as removing asm.js.
(In reply to Tom Schuster [:evilpie] from comment #2)
> > The question is whether this is perf-neutral, or whether valueOf should be
> > inlined using JS_INLINABLE_FN. The ToObject will be inlined no matter what,
> > but that still leaves valueOf potentially not being inlined if the inlining
> > depth is exhausted. It also seems trivial to inline it.
> > 
> I mean before this was always a native call, so probably not the fastest
> thing. But you are right this is simple enough to inline that we should just
> do it.

Great. You're right that I'm needlessly bemoaning an imperfection in something that's substantially superior to what we had before, though, sorry.

> I think what you are getting is that the name isn't perfect and I agree. The
> idea is to check for specific builtin method, which thus far has been native
> (built-in) methods. We just know that these methods have no side-effects and
> thus this hack should not be observable. My general idea of putting this
> inside AsmJS.cpp was the slim hope that we wouldn't need this anywhere else
> and could remove it at the same time as removing asm.js.

I guess maybe that's what I'm saying? I had kinda assumed that there might be more cases where we'd need this and HasNativeMethodPure, in which case it'd make sense to unify them so they can be found easier. But if that's not the case, leaving it in AsmJS.cpp makes perfect sense.
(In reply to Till Schneidereit [till] from comment #3)
> (In reply to Tom Schuster [:evilpie] from comment #2)
> > > The question is whether this is perf-neutral, or whether valueOf should be
> > > inlined using JS_INLINABLE_FN. The ToObject will be inlined no matter what,
> > > but that still leaves valueOf potentially not being inlined if the inlining
> > > depth is exhausted. It also seems trivial to inline it.
> > > 
> > I mean before this was always a native call, so probably not the fastest
> > thing. But you are right this is simple enough to inline that we should just
> > do it.
> 
> Great. You're right that I'm needlessly bemoaning an imperfection in
> something that's substantially superior to what we had before, though, sorry.
> 
I am filing a followup for this, but will self-assign.
> > I think what you are getting is that the name isn't perfect and I agree. The
> > idea is to check for specific builtin method, which thus far has been native
> > (built-in) methods. We just know that these methods have no side-effects and
> > thus this hack should not be observable. My general idea of putting this
> > inside AsmJS.cpp was the slim hope that we wouldn't need this anywhere else
> > and could remove it at the same time as removing asm.js.
> 
> I guess maybe that's what I'm saying? I had kinda assumed that there might
> be more cases where we'd need this and HasNativeMethodPure, in which case
> it'd make sense to unify them so they can be found easier. But if that's not
> the case, leaving it in AsmJS.cpp makes perfect sense.
HasNativeMethodPure is used in a pretty fixed set of places that I hope we won't increase in the future, but I will keep eye on this.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/691c601b38ec
Self-host Object.prototype.valueOf. r=till
Blocks: 1334251
https://hg.mozilla.org/mozilla-central/rev/691c601b38ec
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.