Closed Bug 552574 Opened 14 years ago Closed 14 years ago

TM: de-mystify js_ValueToNumber API

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: gal, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

with the signature change
76.00 KB, patch
igor
: review+
Details | Diff | Splinter Review 
js_ValueToNumber is called with a jsval *vp and returns a double. Errors are indicated by setting *vp to JSVAL_NULL, instead of the usual boolean flag. This is very non-obvious and its a rite of passage for people starting to hack on SM to run into this (I did, vlad just did).

The signature should look as follows:

bool js_ValueToNumber(JSContext *cx, jsval *vp, jsdouble *dp)

If we are worried that this is on a hot path (doesn't seem so, at least not with tracer on), we can inline relevant portions of it into an inline function and leave the rest in jsnum.cpp.

Also, we should either not touch *vp, or always leave it with a valid value. Right now if we return a double, we don't put the equivalent JSVAL into *vp, but we might use vp as a root and destroy it. defaultValue() is rare enough that we should use an AutoRooter and just leave vp alone. Errors are signaled through the return value.
Minor note: ideally this would just not touch *vp with the new signature.  Allocating a new jsval here would be unfortunate.
(In reply to comment #0)
> If we are worried that this is on a hot path (doesn't seem so, at least not
> with tracer on), we can inline relevant portions of it into an inline function
> and leave the rest in jsnum.cpp.

The signature was changed to the current one since it was on some hot paths in the interpreter. Thus inlining JSVAL_DOUBLE and JSVAL_INT cases should be considered.
Please do not ignore or downplay past profile-driven optimization, alluded to by bug 550351 comment 37 and now comment 2. A little cvs annotate finds the story: bug 421154. Maybe it's not a big deal now, but it was a win then.

Agree the signature is confusing, though! Want to fix that without perf loss.

/be
Absolutely not. I am not looking to lose perf here. I am convinced inlining is superior or at least equivalent in performance to what we have right now. We should not trade strange signatures for perf wins we can have with a simple signature, too, that's all.
I was also interested in simplifying js_ValueToNumber.  It seems that there are two interests served by this 1 function:
 1. callers that test *vp == JSVAL_NULL and then use the returned double
 2. callers that also use *vp == JSVAL_TRUE as a faster version of (JSVAL_IS_INT(*vp) || JSVAL_IS_DOUBLE(*vp)).

Case 1 would be penalized if a new double was allocated for *vp, since they wouldn't use it.  Case 2 would be (slightly) penalized *vp was changed to the above signature, since they would need to do a slower test to determine whether they needed to js_NewNumberInRootedValue.  From a quick scan, most callers are case 1, 5 (hot) callers are case 2: (js_DoIncDec, Number, js_ValueToInt32, js_ValueToUint16, JSOP_POS).

So that suggests two functions: one with the signature Andreas suggested and one for psychopaths.  With a bit of template lovin', we could even share the code.
Luke: Brilliant! Split cases to decomplexify the overloaded octopus connection (for one case anyway) and keep optimizing, while sharing code with templates. You gonna take this bug?

/be
You betcha.
Assignee: general → lw
Further supporting the need for this change, the first use of js_ValueToUint16 in str_fromCharCode seems to be missing a check for JSVAL_IS_NULL.

js> o = { valueOf:function(){return {}} }
({valueOf:(function () {return {};})})
js> String.fromCharCode(o)
"\0"

File a new bug?
(In reply to comment #8)
> Further supporting the need for this change, the first use of js_ValueToUint16
> in str_fromCharCode seems to be missing a check for JSVAL_IS_NULL.
> 
> js> o = { valueOf:function(){return {}} }
> ({valueOf:(function () {return {};})})
> js> String.fromCharCode(o)
> "\0"
> 
> File a new bug?

No, this is correct, see ES5

9.7 ToUint16

1. Let number be the result of calling ToNumber on the input argument.
2. If number is NaN, +0, 0, +, or , return +0.

and above that

9.3.1 ToNumber Applied to the String Type

ToNumber applied to Strings applies the following grammar to the input String. If the grammar cannot interpret the String as an expansion of StringNumericLiteral, then the result of ToNumber is NaN.

/be
(In reply to comment #9)
Oops; the issue is that fromCharCode ignores errors from its ValueToNumber call. (I thought valueOf returning a non-primitive was an error.)  Perhaps a more clear example:

js> o = { valueOf:function(){throw 1} }
js> String.fromCharCode(o)
"\0"
Yes, that's the one to file. Thanks,

/be
Filed bug 555484.
Attached patch FAIL (obsolete) — — Splinter Review 
After trying a bit, it seems that (1) the signature
  (double (*)(JSContext *, jsval *)
is hard to beat and (2) that the js_ValueTo{Number,ECMAInt,Int} family is called enough to matter.  The attached patch uses
  (bool (*)(JSContext *, jsval *, double *)
for case 1 described in comment 5 and, even though shark shows that less execution time is spent in the body of js_ValueToNumber, SS/V8 are 1-4% slower on x86 and x64.  Looking at the dissassembly for a microbenchmark, I can see more code generated for callsites of this signature, so I think the fact that it (and the rest of the js_ValueTo* crew) are called from 1000 places (esp. in js_Interpret) adds up.

I also tried sticking with the same same signature and, instead of returning failure with *vp == JSVAL_NULL, returning a NaN-boxed magic value.  Unfortunately, a simple microbenchmark shows that this also generates more code for callsites on x86 and x64 and runs slower on x86.

For the js_ValueTo*Int functions, I also tried:
union U {
  int64_t wide;
  struct { int32_t value, error; };
  U(int64_t w) : wide(w) {}
};
int64_t js_ValueToNumber(JSContext *, jsval *vp);

which lets you write:

U u = js_ValueToNumber(cx, vp);
if (u.error)
  goto error;
... u.value

the idea being that the return value and error go through registers.  This is what happens on x64, and it runs faster in a microbenchmark.  However, for x86, GCC really really wants to put that return value on the stack (after it has been returned through eax/edx), so it goes slower.  I was not able to convince it otherwise with various casts, consts, and scopes.

I can still see the value of making a less-than-optimal version with the cleaner signature, but I've lost a little of my excitement for the whole thing :|
Igor did indeed optimize. Maybe we can't split cases but could we templatize for cleaner and more consistent (less human error opportunity) error checking?

/be
The cleaner signature in the patch I posted would catch more errors; it would make sense to reign in the number of places where I switched to that signature until it stopped being a measurable loss.  Already, the patch does that for the case 2 callers mentioned in comment 5.
Let's use that spanky templatized internal API, then -- without perf hit we still get defense against bugs such as the fromCharCode one you spotted, right?

/be
> union U {
>   int64_t wide;
>   struct { int32_t value, error; };
>   U(int64_t w) : wide(w) {}
> };
> int64_t js_ValueToNumber(JSContext *, jsval *vp);
> 
> which lets you write:
> 
> U u = js_ValueToNumber(cx, vp);
> if (u.error)
>   goto error;
> ... u.value
> 
> the idea being that the return value and error go through registers.  This is
> what happens on x64, and it runs faster in a microbenchmark.  However, for x86,
> GCC really really wants to put that return value on the stack (after it has
> been returned through eax/edx), so it goes slower.

What is the call site where this happens?
(In reply to comment #16)
Maybe I'm missing your point or I gave the wrong impression earlier; the only template I used was js::GenericNumberToValue<>, which is an implementation detail used to implement:
  bool js::NumberToValue(JSContext *, jsval *, double *)  // new, clean
  bool js::NumberToValueFancy(JSContext *, jsval *)       // old, mystic

(In reply to comment #17)
> What is the call site where this happens?

In my microbenchmark, the code:

        { const U r = function_returning_packed_int64(pi);
          if (r.error)
              goto error;
          sum += r.value; }

generates:

 mov    %esi,(%esp)
 call   8048720 <function_returning_packed_int64>
 test   %eax,%eax          // good
 mov    %eax,-0x30(%ebp)   // yes, please, store back to this variable,
 mov    %edx,-0x2c(%ebp)   // just in time for it to go out of scope
 jne    8048878
 add    %edx,%ebx          // good

I don't know why it does those stores; maybe compilers think of unions like we think of evals.
Hah!  I forgot that I gave U a convenience constructor.  Despite not being called, it seems even the ghost of its call is enough to flag 'r' as escaping.  Removing it and writing:

  U r = { function_returning_packed_int64(pi) };

removes the loads I was complaining about and makes the packed return version ~5% faster than returning through an outparam.  (Only 5%! -- it would seem that if you have 100% L1 hits, loads are fast.)  So that's cool.

With the packed return, gcc on x64 generates:

 movq   %r12,%rdi
 callq  <function_returning_packed_int64>
 movq   %rax,%rdx
 shrq   $0x20,%rax
 testl  %eax,%eax
 jne    <error>
 addl   %edx,%ebx

thereby replacing the a load with a shrq.  In the microbench, this is ~2% slower that indicating failure via outparam (again with the fast 100%-L1 loads!).

So, all in all, if the syntax:

 Uint32AndError r = { js_ValueToECMAUint32(cx, vp) };
 if (r.error)
   ...
 ... = r.value;

isn't too painful, there is no real penalty for using the safer signature for the js_ValueTo*Int functions.  I guess I should also check MSVC...
The {} are pretty painful, though maybe we can hide them with some macro love?  Also should check to see what this does on ARM.
MSVC x86 does the same as GCC and packed-return is as fast as the outparam.  On the Pegatron ARM boxes, packed-return is 60% faster than the outparam.  So we good.

(In reply to comment #20)
> The {} are pretty painful, though maybe we can hide them with some macro love? 

#define JS_VALUE_TO_ECMA_UINT32(cx,vp) { js_ValueToECMAUint32(cx, vp) }
Uint32AndError r = JS_VALUE_TO_ECMA_UINT32(cx, vp);

This look ok to everyone?
(In reply to comment #19)
>  Uint32AndError r = { js_ValueToECMAUint32(cx, vp) };

What about using a couple of helper methods like:

inline bool UnpackError(uint64 x) { return !!(x >> 32); }
inline unsigned UnpackUint32(uint64 x) { return unsigned(x); }

with a usage like:

uint64 x = js_ValueToECMAUint32(cx, v);
if (UnpackError(x))
    return false;

unsigned i = UnpackResult(x);
I would much rather prefer inline methods on the return type.
(In reply to comment #23)
> I would much rather prefer inline methods on the return type.

GCC cannot return 64 bit struct in registers on x86, it can only do that apparently for 64 bit primitive types. Thus js_ValueToECMAUint32 needs to return uint64 or something. 

But we can try class on the return side. The class would provide unwrap methods. I just checked that the following source with GCC 4.4 generates the same optimized code with the result in registers and no extra temporaries for both do_something (helper unwrap methods) and do_something2 (helper unwrap class):

~/s> cat x.cpp
typedef unsigned long long uint64;

inline bool UnpackError(uint64 x) { return !!(x >> 32); }
inline unsigned UnpackUint32(uint64 x) { return unsigned(x); }

struct Unwrap {
    uint64 x;

    Unwrap(uint64 x) : x(x) {}

    bool error() const { return !!(x >> 32); }

    unsigned value() const { return unsigned(x); }
};


extern unsigned long long
js_ValueToECMAUint32(void *cx, long val);

bool
do_something(void *cx, long val, unsigned *result)
{
    unsigned long long x = js_ValueToECMAUint32(cx, val);
    if (UnpackError(x))
        return false;
    *result = UnpackUint32(x) * 177;
    return true;
}

bool
do_something2(void *cx, long val, unsigned *result)
{
    Unwrap x(js_ValueToECMAUint32(cx, val));
    if (x.error())
        return false;
    *result = x.value() * 177;
    return true;
}

    
~/s> gcc -S -O3 -fno-exceptions x.cpp
~/s> cat x.s
	.file	"x.cpp"
	.text
	.p2align 4,,15
.globl _Z13do_something2PvlPj
	.type	_Z13do_something2PvlPj, @function
_Z13do_something2PvlPj:
	pushl	%ebp
	movl	%esp, %ebp
	subl	$24, %esp
	movl	12(%ebp), %eax
	movl	%eax, 4(%esp)
	movl	8(%ebp), %eax
	movl	%eax, (%esp)
	call	_Z20js_ValueToECMAUint32Pvl
	xorl	%ecx, %ecx
	testl	%edx, %edx
	jne	.L3
	imull	$177, %eax, %eax
	movl	16(%ebp), %edx
	movl	$1, %ecx
	movl	%eax, (%edx)
.L3:
	movl	%ecx, %eax
	leave
	ret
	.size	_Z13do_something2PvlPj, .-_Z13do_something2PvlPj
	.p2align 4,,15
.globl _Z12do_somethingPvlPj
	.type	_Z12do_somethingPvlPj, @function
_Z12do_somethingPvlPj:
	pushl	%ebp
	movl	%esp, %ebp
	subl	$24, %esp
	movl	12(%ebp), %eax
	movl	%eax, 4(%esp)
	movl	8(%ebp), %eax
	movl	%eax, (%esp)
	call	_Z20js_ValueToECMAUint32Pvl
	xorl	%ecx, %ecx
	testl	%edx, %edx
	jne	.L8
	imull	$177, %eax, %eax
	movl	16(%ebp), %edx
	movl	$1, %ecx
	movl	%eax, (%edx)
.L8:
	movl	%ecx, %eax
	leave
	ret
	.size	_Z12do_somethingPvlPj, .-_Z12do_somethingPvlPj
	.ident	"GCC: (GNU) 4.4.3 20100226 (Red Hat 4.4.3-8)"
	.section	.note.GNU-stack,"",@progbits
This has nothing to do with gcc, its an ABI matter:

64-bit:

typedef struct foo {
  int a;
  int b;
} foo;

extern foo hello();

int y() {
  foo y = hello();
  return y.a + y.b;
}

	call	_hello
	movq	%rax, %rdx
	sarq	$32, %rdx
	addl	%edx, %eax
	leave
	ret

32-bit:

	movl	%esp, %ebp
	subl	$8, %esp
	call	_hello
	addl	%edx, %eax
	leave
	ret

So this works very nicely with the Mac ABI. If it works with MSVC, I would prefer to go with this and not uglify the source for the sake of Linux. Also, we should consider making js_ValueToNumber inline, in which case even for Linux this might get deconstructed a little.
To my great displeasure, MSVC refuses to pass struct-typed parameters through registers so I assumed that they wouldn't do any better for return types.  But I was wrong -- I just checked and indeed they do.  So that seems like the way to go for the js_ValueTo*int functions; I was not feeling great about any of the other choices.

As for the inlining, it looks like js_Interpret already manually inlines the fast path for int/double calls to js_ValueTo*.  Perhaps the cleaner thing to do is remove these and split the js_ValueTo* calls into an inline call that handles the ints/doubles and calls a non-inline call for the general case.  I'll try that.
Attached patch patch (obsolete) — — Splinter Review 
I took out the two instantiations of GenericValueToNumber<> and just implemented ValueToNumber in terms of ValueToNumberFancy for a small perf boost (despite the fact that this does a little more work in the non-Fancy case -- icache effects I suppose).  I also made inline fast paths for JSVAL_INT that call a non-inline *Slow version.  Benchmarking on SS/V8 shows frustratingly arbitrary performance effects: for a while I was seeing overall <1% slowdown for some OS/arch/test triples (with individual tests varying from +5% to -5%), but that all changed to breaking even (or slightly speeding up) after rebasing to tip.  Butterfly wings and hurricanes.
Attachment #435437 - Attachment is obsolete: true
Attachment #436576 - Flags: review?(igor)
Blocks: 555484
Attachment #436576 - Attachment is patch: true
Attachment #436576 - Attachment mime type: application/octet-stream → text/plain
Luke: have you tried just to inline fast paths of ValueToSomething while making their signature like it was proposed in the comment 0 or even like 

bool ValueToNumber(JSContext *cx, jsval vp, jsdouble *dp)
bool ValueToInt32(JSContext *cx, jsval vp, jsint *ip)
...

and replacing the custom inlining in the interpreter with the new functions?
Attached patch v2 (obsolete) — — Splinter Review 
That's a good point about the inlining making the signature different not matter, so this patch uses the signature from comment 0.  It is .6% faster (significant) on SS/Linux than trunk.  For everything else, though, it tends to be a tad bit slower (< 1%, but significant).  I'm getting the general feeling that most of these performance changes are not so much direct effects of the change but secondary effects of changing things like where code sits in memory.
Attachment #436773 - Flags: review?(igor)
(In reply to comment #29)
> Created an attachment (id=436773) [details]
> v2
> 
> That's a good point about the inlining making the signature different not
> matter, so this patch uses the signature from comment 0.

Could you also try to use a signature that takes jsval, not jsval *? That signature would be more in line with other value converters like js_ValueToString(JSContext *cx, Jsval v) or js_ValueToObject(JSContext *cx, jsval v, JSObject **objp). Given that inlining takes care about performance difference it is better to use the same pattern everywhere.
(In reply to comment #30)
I left it this way for the access to a rooted jsval, but a second AutoValueRooter could be used for the defaultValue call.  (Technically, I guess this wouldn't be necessary, since the callers are going to be passing a copy of a rooted jsval, but I guess the contract is simpler to not require this; does this happen anywhere else?)
(In reply to comment #31)
> (In reply to comment #30)
> I left it this way for the access to a rooted jsval, but a second
> AutoValueRooter could be used for the defaultValue call.  (Technically, I guess
> this wouldn't be necessary, since the callers are going to be passing a copy of

s/copy of/pointer to/

> a rooted jsval, but I guess the contract is simpler to not require this; does
> this happen anywhere else?)

Pronoun trouble -- if by "this" you mean jsval *vp formal parameter is passed down without redundant tvr-based (or other) rooting, then yes, this happens in other places. It's important not to waste cycles rooting already-rooted values.

We lack the static analysis to prove the outermost call in any such chain does in fact root, but we (a) audit in lieu of that; (b) should get that analysis done (bug 353231). Or how about (c): if there is a cheap-enough C++ type-based way to prove caller-roots, then please propose it in a new bug.

/be
(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > I left it this way for the access to a rooted jsval, but a second
> > AutoValueRooter could be used for the defaultValue call.  (Technically, I guess
> > this wouldn't be necessary, since the callers are going to be passing a copy of
> 
> s/copy of/pointer to/

I did mean 'copy of': IIUC, Igor was suggesting:

  bool js_ValueToNumber(JSContext *, jsval v, double *);

and I was saying that v was going to be (in all current calls) a copy of a rooted value, hence it was *technically* unnecessary to root v for the defaultValue call, and this could be made part of the calling contract.  However, I was thinking that pass-by-value-but-have-it-rooted-elsewhere was abnormal in the engine and probably future-hostile and thus I should probably go ahead and (redundantly) AutoTemp-root v before this call.
Caller-roots still applies.

/be
(In reply to comment #33)
>   bool js_ValueToNumber(JSContext *, jsval v, double *);
> 
> and I was saying that v was going to be (in all current calls) a copy of a
> rooted value, hence it was *technically* unnecessary to root v for the
> defaultValue call, and this could be made part of the calling contract.

You do not need to root v as indeed it should be rooted by the caller. In jsapi.cpp there are number of places where an explicit autoroot is used but this is due to a possible violation of that rule.
 
> However, I was thinking that pass-by-value-but-have-it-rooted-elsewhere was
> abnormal in the engine

No: in fact the rule that the caller roots was proposed among other rulers for the static checker.
Attached patch pass by value (obsolete) — — Splinter Review 
Great, that simplifies things.  This patch passes values by value and uses ValueToNumberFancy for the few cases where the caller wants *vp as an outparam.
Attachment #436576 - Attachment is obsolete: true
Attachment #436773 - Attachment is obsolete: true
Attachment #437072 - Flags: review?(igor)
Attachment #436576 - Flags: review?(igor)
Attachment #436773 - Flags: review?(igor)
Comment on attachment 437072 [details] [diff] [review]
pass by value

>@@ -853,18 +851,18 @@ exn_toSource(JSContext *cx, uintN argc, 
>-        lineno = js_ValueToECMAUint32 (cx, &localroots[2]);
>-        if (JSVAL_IS_NULL(localroots[2]))
>+        uint32_t lineno;
>+        if (!ValueToECMAUint32 (cx, localroots[2], &lineno))
>             return false;

Nit: remove pre-existing space between ValueToECMAUint32 and ().

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -1506,18 +1506,17 @@ js_DoIncDec(JSContext *cx, const JSCodeS
>     jsdouble d;
> 
>     v = *vp;
>     if (JSVAL_IS_DOUBLE(v)) {
>         d = *JSVAL_TO_DOUBLE(v);
>     } else if (JSVAL_IS_INT(v)) {
>         d = JSVAL_TO_INT(v);
>     } else {
>-        d = js_ValueToNumber(cx, vp);
>-        if (JSVAL_IS_NULL(*vp))
>+        if (!ValueToNumberFancy(cx, vp, &d))
>             return JS_FALSE;
>         JS_ASSERT(JSVAL_IS_NUMBER(*vp) || *vp == JSVAL_TRUE);

Preexisting nit: Comment before the first if that js_ValueToNumber is not used to avoid boxing the double result twice.

> /*
>  * Optimized conversion macros that test for the desired type in v before
>  * homing sp and calling a conversion function.
>  */
> #define VALUE_TO_NUMBER(cx, n, v, d)                                          \
>     JS_BEGIN_MACRO                                                            \
>         JS_ASSERT(v == regs.sp[n]);                                           \
>         if (JSVAL_IS_INT(v)) {                                                \
>             d = (jsdouble)JSVAL_TO_INT(v);                                    \
>         } else if (JSVAL_IS_DOUBLE(v)) {                                      \
>             d = *JSVAL_TO_DOUBLE(v);                                          \
>         } else {                                                              \
>-            d = js_ValueToNumber(cx, &regs.sp[n]);                            \
>-            if (JSVAL_IS_NULL(regs.sp[n]))                                    \
>+            if (!ValueToNumber(cx, regs.sp[n], &d))                           \
>                 goto error;                                                   \
>-            JS_ASSERT(JSVAL_IS_NUMBER(regs.sp[n]) ||                          \
>-                      regs.sp[n] == JSVAL_TRUE);                              \
>         }                                                                     \
>     JS_END_MACRO

The comment before the macro have been not applicable for quite some time since the introduction of the regs structure that "homes" sp automatically. Thus remove the comments and replace the macro body with ValueToNumber call.

>diff --git a/js/src/jsnum.h b/js/src/jsnum.h
>+JS_ALWAYS_INLINE bool
>+ValueToNumber(JSContext *cx, jsval v, double *out)
>+{
>+    if (JSVAL_IS_INT(v)) {
>+        *out = JSVAL_TO_INT(v);
>+        return true;
>+    }
>+    if (JSVAL_IS_DOUBLE(v)) {
>+        *out = *JSVAL_TO_DOUBLE(v);
>+        return true;
>+    }
>+    extern double ValueToNumberSlow(JSContext *, jsval *);
>+    jsval param = v;
>+    *out = ValueToNumberSlow(cx, &param);
>+    return !JSVAL_IS_NULL(param);

Do not introduce GC-hazard-friendly pattern of passing an address of unrooted jsval here. Thus lets make the slow version to use the same signature as ValueToNumber uses:

    /* 
     * Use temporary not to leak the out pointer which may prevent the
     * compiler to use the register when inlining.
     */ 
    extern bool ValueToNumberSlow(JSContext *, jsval v, double *out);
    jsdouble tmp;
    if (!ValueToNumberSlow(cx, v, &tmp))
        return false;
    *out = tmp;
    return true;


>+JS_ALWAYS_INLINE bool
>+ValueToNumberFancy(JSContext *cx, jsval *vp, double *out)

Look at the usage pattern of ValueToNumberFancy in JSOP_POS, the Number constructor and js_DoIncDec helper function. In the first two cases the code do not need  the double value at all. To cover those cases what is necessary is a function like bool js_EnsureNumber(cx, jsval *vp) that makes *vp to be either int or double jsval. For an added benefit it will avoid unnecessary js_NewNumberInRootedValue in those cases when converting an object.

Now, js_DoIncDec does need both the double value and the jsval but only in the post case. This suggests  to implement the post and pre cases differently using js_EnsureNumber for the post and ValueToumber for pre like in:

JS_STATIC_INTERPRET JSBool
js_DoIncDec(JSContext *cx, const JSCodeSpec *cs, jsval *vp, jsval *vp2)
{
    if (cs->format & JOF_POST) {
        if (!EnsureNumber(cx, vp))
            return false;
	jsdouble d = JSVAL_IS_INT(*vp) ? JSVAL_TO_INT(*vp) : JSVAL_TP_DOUBLE(*vp);
        (cs->format & JOF_INC) ? d++ : d--;
        return js_NewNumberInRootedValue(cx, d, vp2);
    }

    jsdouble d;
    if (!ValueToNumber(cx, vp, &d))
        return false;
    (cs->format & JOF_INC) ? d++ : d--;

    if (!js_NewNumberInRootedValue(cx, d, vp2))
        return false;
    *vp = *vp2;
    return true;
}

One can even try to replace js_DoIncDec with explicit js_DoPreIncDec and js_DoPostIncDec, but that can wait another bug.

>+JS_ALWAYS_INLINE bool
>+ValueToECMAInt32(JSContext *cx, jsval v, int32_t *out)
>+{
>+    if (JSVAL_IS_INT(v)) {
>+        *out = JSVAL_TO_INT(v);
>+        return true;
>+    }
>+    extern int32_t ValueToECMAInt32Slow(JSContext *, jsval *);
>+    jsval param = v;

We need to investigate if it is worth to add an a fast case for double representing int32 that does not fit int jsval and similarly for ValueToECMAUint32. I.e. something like:

    if (JSVAL_IS_INT(v)) {
        *out = JSVAL_TO_INT(v);
        return true;
    }
    if (JSVAL_IS_DOUBLE(v)) {
        double d = *JSVAL_TO_DOUBLE(v);
        int32_t i = int32_t(d);
        if (double(i) == d) {
            *out = i;
            return true;
        }
    }

>diff --git a/js/src/jsops.cpp b/js/src/jsops.cpp
> END_CASE(JSOP_URSH)
>@@ -1109,35 +1109,31 @@ BEGIN_CASE(JSOP_NEG)
>         JS_STATIC_ASSERT(!INT_FITS_IN_JSVAL(-JSVAL_INT_MIN));
>         i = -i;
>         JS_ASSERT(INT_FITS_IN_JSVAL(i));
>         regs.sp[-1] = INT_TO_JSVAL(i);
>     } else {
>         if (JSVAL_IS_DOUBLE(rval)) {
>             d = *JSVAL_TO_DOUBLE(rval);
>         } else {
>-            d = js_ValueToNumber(cx, &regs.sp[-1]);
>-            if (JSVAL_IS_NULL(regs.sp[-1]))
>-                goto error;
>-            JS_ASSERT(JSVAL_IS_NUMBER(regs.sp[-1]) ||
>-                      regs.sp[-1] == JSVAL_TRUE);
>+            if (!ValueToNumber(cx, regs.sp[-1], &d))
>+                goto error;

Remove the JSVAL_IS_DOUBLE(rval) case and use js_ValueToNumber for that as well assuming that either the compiler can eliminate the redundant JSVAL_IS_INT check or that the CPU will predict the jump around this case.

>@@ -946,18 +943,17 @@ str_charAt(JSContext *cx, uintN argc, js
>         if ((size_t)i >= str->length())
>             goto out_of_range;
>     } else {
>         NORMALIZE_THIS(cx, vp, str);
> 
>         if (argc == 0) {
>             d = 0.0;
>         } else {
>-            d = js_ValueToNumber(cx, &vp[2]);
>-            if (JSVAL_IS_NULL(vp[2]))
>+            if (!ValueToNumber(cx, vp[2], &d))
>                 return JS_FALSE;
>             d = js_DoubleToInteger(d);


This pattern, ValueToNumber/DoubleToInteger is too widespread and deserves its own method like ValueToInteger. But that can wait another bug.

Given the amount of the suggested chnages the patch would need another review.
Attachment #437072 - Flags: review?(igor)
Two questions:

> Preexisting nit: Comment before the first if that js_ValueToNumber is not used
> to avoid boxing the double result twice.

I'm sorry, I wasn't quite able to understand this sentence, could you expand?

> >+    extern double ValueToNumberSlow(JSContext *, jsval *);
> >+    jsval param = v;
> >+    *out = ValueToNumberSlow(cx, &param);
> >+    return !JSVAL_IS_NULL(param);
> 
> Do not introduce GC-hazard-friendly pattern of passing an address of unrooted
> jsval here.

True (which is why I documented the hazard at the top of each *Slow function) but the trickiness is encapsulated inside the ValueTo*{,Slow} functions, so it seems like we could have our cake (faster signature) and eat it too (simple API for callers).  I can change it, though, if you think the hazard is still serious in spite of this reasoning.
(In reply to comment #38)
> Two questions:
> 
> > Preexisting nit: Comment before the first if that js_ValueToNumber is not used
> > to avoid boxing the double result twice.
> 

I meant here that js_ValueToNumber gives just a number, not jsval. For post-increment that jsval is necessary. As such straight ValueToNumber for cases like converting an object into a double would first generate boxed double, then unbox it to return the number, and then the code would box it again.  

> > Do not introduce GC-hazard-friendly pattern of passing an address of unrooted
> > jsval here.
> 
> True (which is why I documented the hazard at the top of each *Slow function)
> but the trickiness is encapsulated inside the ValueTo*{,Slow} functions, so it
> seems like we could have our cake (faster signature) and eat it too (simple API
> for callers).  I can change it, though, if you think the hazard is still
> serious in spite of this reasoning.

The rules could be relaxed for performance critical code, but this is not the case here at all. So lets avoid passing pointer to an unrooted location even when it is hidden behind a layer of another API. As an added benefit it would keep the interface uniform everywhere.
Attached patch with changes (obsolete) — — Splinter Review 
Good points.  This patch replaces ValueToNumberFancy with:

  bool ValueToNumberValue(JSContext*, jsval *, double *);
  bool ValueToNumberValue(JSContext*, jsval *);

which do what you suggested with EnsureNumber.  The former is used by js_DoIncDec (to avoid branching on JSVAL_IS_INT(*vp)) and the latter by Number() and JSOP_POS.
Attachment #437072 - Attachment is obsolete: true
Attachment #437352 - Flags: review?(igor)
Also, I was able to change all the ValueTo*Int*Slow signatures to match their non-Slow counterparts, however, ValueToNumberSlow genuinely needs vp as an outparam, so I left it the way it is, with comment.
Attachment #437352 - Flags: review?(igor)
Comment on attachment 437352 [details] [diff] [review]
with changes

val
>+double
>+ValueToNumberSlow(JSContext *cx, jsval *vp)
> {
>+    /* N.B. a copy of *vp is rooted elsewhere; vp may not be a rooted jsval. */

This patch still uses a GC-hazard-frienfly signature on a cold code path. Please fix this.

>+JS_ALWAYS_INLINE bool
>+ValueToNumberValue(JSContext *cx, jsval *vp, double *out)
>+{
...
>+    if (JSVAL_IS_NULL(v) ||
>+        (v == JSVAL_TRUE && !js_NewNumberInRootedValue(cx, d, vp))) {

This is suboptimal as for object->number conversion it will first unbox the number and then box it again in js_NewNumberInRootedValue. In addition it bloats the inlined code with two function calls on the slow path, first to convert the number and then to box it. This suggests to have a special slow method for ValueToNumberValue. But this optimization can be done in another bug. So assume r+ with that signature fixed.
(In reply to comment #42)
> This is suboptimal as for object->number conversion it will first unbox the
> number and then box it again in js_NewNumberInRootedValue.

I don't see where this happens on the object->number path: defaultValue will fill its outparam with a value, thus v will not be JSVAL_TRUE and  js_NewNumberInRootedValue will not be called.

> In addition it
> bloats the inlined code with two function calls on the slow path, first to
> convert the number and then to box it.

True.  I this was fine since, before this change, it was already, literally, inline.  But taking cold code off the hot path sounds good.
While fixing up ValueToNumberSlow, I remembered that part of the defaultValue contract is that the callee may not assume that vp is rooted.  E.g., js_ValueToString, js_ValueToCharBuffer, js_ValueToObject, and ToXMLString pass the address of an unrooted jsval.  As long as this assumption is being made, it makes sense to avoid the AutoValueRooter here too, which this patch does.  Yes?
Attachment #437352 - Attachment is obsolete: true
Attachment #437422 - Flags: review?(igor)
(In reply to comment #44)
> Created an attachment (id=437422) [details]
> with the signature change
> 
> While fixing up ValueToNumberSlow, I remembered that part of the defaultValue
> contract is that the callee may not assume that vp is rooted.

It is OK to assume that for now so we can proceed and land this patch. But we need a follow up to address that.
Attachment #437422 - Flags: review?(igor) → review+
(In reply to comment #43)
 I don't see where this happens on the object->number path: defaultValue will
> fill its outparam with a value, thus v will not be JSVAL_TRUE and 
> js_NewNumberInRootedValue will not be called.

Yes, you are right, the patch does everything properly.
Thanks for all the help Igor
http://hg.mozilla.org/tracemonkey/rev/2205abaf380c
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/2205abaf380c
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: