749786 - prototype int64/uint64 value objects, with operators and literal syntax

Status: RESOLVED DUPLICATE of bug 1366287

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Patch coming along, hope to post it later today.

/be

Comment 1

[Brendan Eich [:brendan]]

Doing only int64 and uint64 to start.

/be

Comment 2

[Dave Herman [:dherman]]

Brendan: thanks for prototyping this. Can you include these two features in your implementation?

- a predicate to test whether an object is a value object: Object.isValue
- a check on WeakMap to disallow the use of value objects as keys

Thanks,
Dave

Comment 3

[Brendan Eich [:brendan]]

(In reply to Dave Herman [:dherman] from comment #2)
> - a predicate to test whether an object is a value object: Object.isValue

Will do, that's in the strawman already.

> - a check on WeakMap to disallow the use of value objects as keys

Could you please update the strawman to talk about this, including rationale? Thanks,

/be

Comment 4

[Dave Herman [:dherman]]

> > - a predicate to test whether an object is a value object: Object.isValue
> 
> Will do, that's in the strawman already.

OK. You asked on IRC what Object.isValue should return for non-object values. My first reaction was throw, since it's meant to be for objects. But I can't come up with another name that's not completely stilted:

    isSynthetic
    isDeep
    isStructural
    isExtensional
    isForgeable
    isSynthetic

Meh. Probably the most straightforward thing is for Object.isValue to return true for all non-object values and all non-reference objects, and false for all reference objects.

Even though I loathe the terminology of "value object." What can you do, I wrote the spec. ;-P

> > - a check on WeakMap to disallow the use of value objects as keys
> 
> Could you please update the strawman to talk about this, including
> rationale? Thanks,

Added.

Dave

Comment 5

[Dave Herman [:dherman]]

What are you doing about the coercion rules for the various operators? Just kinda winging it, or leaving it out completely for the first cut? (I haven't had a chance to go through the spec and work out the details yet.)

Dave

Comment 6

[Brendan Eich [:brendan]]

Attachment: work in progress, does + on {int64,number}

More soon, sorry for the delay. See the comment in jsclass.h for the operator dispatch design.

/be

Comment 7

[Brendan Eich [:brendan]]

> the most straightforward thing is for Object.isValue to return true for all non-object
> values and all non-reference objects, and false for all reference objects.

This wins, indeed.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to Dave Herman [:dherman] from comment #5)
> What are you doing about the coercion rules for the various operators?

Strictness reigns, some examples:

js> i = int64(1.5)
typein:1: TypeError: can't convert 1.5 to int64

js> i = int64(9223372036854775808)
typein:2: TypeError: can't convert 9223372036854776000 to int64

js> i = int64('9223372036854775808')
typein:3: TypeError: can't convert "9223372036854775808" to int64

js> i = int64('9223372036854775807') 
int64('9223372036854775807'

js> j = int64('-9223372036854775808')
int64(-9223372036854775808)

js> j = int64('-9223372036854775809') 
typein:6: TypeError: can't convert "-9223372036854775809" to int64

js> u = uint64(-1)
typein:7: TypeError: can't convert -1 to uint64

js> u = uint64(18446744073709551616)
typein:8: TypeError: can't convert 18446744073709552000 to uint64

js> u = uint64('18446744073709551616')
typein:9: TypeError: can't convert "18446744073709551616" to uint64

js> u = uint64('18446744073709551615') 
uint64('18446744073709551615')

Working on bitwise binary ops, then on to unaries.

Per the comment in jsclass.h based in Christian Hansen's 2009 proposal, I'm also not (yet) supporting implicit toString via +:

js> i = int64(42)
int64(42)
js> i+9
int64(51)
js> i+'hi'
typein:3: TypeError: no binary operator function found for + use
js> i.toString()
"42"
js> i.toString()+'hi'
"42hi"

Spec comment:

 * When executing the '+' operator in 'A + B' where A and B refer to value
 * objects, do the following:
 *
 *  1. Get the value of property LOP_PLUS in A, call the result P
 *  2. If P is not a list throw a TypeError: no '+' operator
 *  3. Get the property ROP_PLUS in B, call the result Q
 *  4. If Q is not a list give a TypeError: no '+' operator
 *  5. Intersect the lists P and Q, call the result R
 *  6. If R is empty throw a TypeError: no '+' operator
 *  7. If R has more than one element throw a TypeError: ambiguity
 *  8. If R[0], call it F, is not a function throw a TypeError
 *  9. Evaluate F(A, B) and return the result

Two options:

A. Change steps 2 and 4 to fall back on old + behavior rather than throw a TypeError.

B. Add operator functions to String.prototype as well as Number.prototype (and presumably to Boolean.prototype too).

Comments welcome. I'll put a rebased patch up for backup and anyone who wants to play with it.

/be

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: work in progress, more binary ops done in interpreter (bitwise, ===/!==, unary still todo)

Comment 10

[Brendan Eich [:brendan]]

Attachment: WIP: bitwise and shift ops done, fixed silly snprintf buf size bugs

Better:

js> u = uint64("0xFFFFFFFFFFFFFFFF")
uint64('18446744073709551615')
js> u >>> 1
uint64('9223372036854775807')
js> u >> 1
uint64('9223372036854775807')
js> i = int64("0xFFFFFFFFFFFFFFFF")  
typein:5: TypeError: can't convert "0xFFFFFFFFFFFFFFFF" to int64
js> i = int64("0x7FFFFFFFFFFFFFFF") 
int64('9223372036854775807')
js> i <<= 1
int64(-2)
js> i >>> 1
int64('9223372036854775807')
js> i >> 1
int64(-1)

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: WIP, all operators implemented, also 1234L suffix a la C#

js> 1234L
1234L
js> i = 9223372036854775807L  
9223372036854775807L
js> i.toString(16)
"7fffffffffffffff"
js> u = 18446744073709551615L
18446744073709551615L
js> v = ~u
0L
js> u == v
false
js> u === v
false
js> w = ~v
18446744073709551615L
js> u == w
true
js> u === w
true
js> if (v) print('nope')
js> if (u) print('yep')
yep

This shows ~, ==, === working as expected. To preserve the identities listed in jsclass.h, ! and != cannot be overloaded, only ==; ditto > and >=, only < and <=.

Yes, objects are not always truthy -- value objects can be falsy. This required undoing the changes from bug 409476, but in doing so I removed some useless proxy veneer on ToBoolean, and studied its workload a bit (see the comments and metering code, to be turned off before pushing the final patch here).

Moar and uglier macros, oh how I wish templates could take operators as static parameters. Help me, Obi-Wan!

Still, pretty sweet to have int64/uint64 prototyped in SpiderMonkey. Onward in a separate bug to TI and JITting for unboxed high performance!

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: WIP, all operators implemented, also 1234L suffix a la C#

Fixed so int64.prototype and uint64.prototype are big enough to hold useless 0 64-bit values.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: WIP, all operators implemented, also 1234L suffix a la C# (also hex literals)

Hex only, no octal. The only good reason for octal these days (that I know of, and I'm an old octal fan from my DEC days) is Unix file modes -- but those are never gonna be 64 bits :-P.

/be

Comment 14

[Brendan Eich [:brendan]]

Still TODO in this bug's patch:

* tests...
* API (JS_New{I,Ui}nt64Object, value getters)
* redo ctypes/Ctypes.cpp to use builtin/IntUtility.h
* jsreflect.cpp needs updating
* frontend/FoldConstants.cpp ditto
* better js::StrictlyEquals solution than memcmp

/be

Comment 15

[Brendan Eich [:brendan]]

More TODO notes:

* fix ambiguous operator error message/args (no useless native function decompilation)
* verify ToBoolean stats with jits off

/be

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 621528 [details] [diff] [review]
WIP, all operators implemented, also 1234L suffix a la C# (also hex literals)

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

(In reply to Brendan Eich [:brendan] from comment #11)
> Moar and uglier macros, oh how I wish templates could take operators as
> static parameters. Help me, Obi-Wan!

The canonical way would be to pass a function object.  Here we could actually use the C++ stdlib: #include <functional> contains std::{plus, minus, multiplies, bit_and, etc}.

In general, do you think we could kill *all* the big macros added by this patch (not just the one that takes an op) with inline functions?

Random nit: for symmetry with JSObject::{isNumber,isBoolean,isString}, could it be JSObject::isValue?

::: js/src/jsinterp.cpp
@@ +857,5 @@
> +
> +                // FIXME: not deep, no -0/0 equiv, no NaN handling. good enough for int64/uint64.
> +                size_t size = lobj.sizeOfThis();
> +                JS_ASSERT(size == robj.sizeOfThis());
> +                *equal = memcmp(&lobj + 1, &robj + 1, size - sizeof(JSObject)) == 0;

I know this is temporary, but I was wondering if, in the final product, there could be a class ValueObject : public JSObject (where class Int64Object : public ValueObject) and then we could put the value-polymorphic methods (like, for this case, 'equal') on ValueObject.

Comment 17

[Brendan Eich [:brendan]]

More TODO:

* int64 x uint64 binary operators
* http://www.cplusplus.com/reference/std/functional/plus/ etc., per Luke's comment 16

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: WIP, add UL suffix, LxUL and ULxL binary operators

Comment 20

[Brendan Eich [:brendan]]

Attachment: WIP, add UL suffix, LxUL and ULxL binary operators

Rebased (painfully).

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 22

[Brendan Eich [:brendan]]

Attachment: rebased yet again

Thanks to Luke for pointers re: Handle<t>::fromMarkedLocation.

Second patch in mq that de-macroizes aggressively (but not completely -- macros have their uses) coming within the week.

/be

Comment 23

[Brendan Eich [:brendan]]

Attachment: rebased, sad to see nice JSObject methods become static

Comment 24

[nemo]

Nice.  Looking forward to this, as it will eliminate the int64 hacks in emscripten.
Our cross-compile uses 64 bit ints extensively, so we take a huge hit right now, even with the double trick and specialcasing to avoid overflow.

Comment 25

[joran]

Thanks, also looking forward. Anyone on WebKit working on this?

Comment 26

[Brendan Eich [:brendan]]

Attachment: rebased, sorry for the delay

Comment 27

[Brendan Eich [:brendan]]

Attachment: rebased, starting to look at the JITs

Comment 28

[Brendan Eich [:brendan]]

Attachment: rebased, looking at JITs on upcoming trip (only time I have to hack :-/)

Comment 29

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 30

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 31

[Brendan Eich [:brendan]]

Attachment: rebased for 2013

I will try to unify JSCLASS_EMULATES_UNDEFINED (JSC's "masquerades_as" is longer but a better trope given the deception :-P) with JSCLASS_VALUE_OBJECT.

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 33

[Brendan Eich [:brendan]]

Attachment: heckuva rebase

Comment 34

[Brendan Eich [:brendan]]

Attachment: rebased yet again

Comment 35

[Brendan Eich [:brendan]]

Attachment: big-ass rebase

Comment 36

[Brendan Eich [:brendan]]

Attachment: copy/paste fix to last rev

Comment 37

[Brendan Eich [:brendan]]

Attachment: rebased past arrow functions (yay!)

Comment 38

[Jim Blandy :jimb]

Comment on attachment 726861 [details] [diff] [review]
rebased past arrow functions (yay!)

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

::: js/src/builtin/Object.cpp
@@ +969,5 @@
> +static JSBool
> +obj_isObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, argc, vp, "Object.isValue", &v))

"Object.isObject" in this one, right?

Comment 39

[Brendan Eich [:brendan]]

Attachment: rebase

Comment 40

[Brendan Eich [:brendan]]

Attachment: rebase-o-rama

Comment 41

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 42

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 43

[Brendan Eich [:brendan]]

Attachment: rebased

Comment 44

[Brendan Eich [:brendan]]

Attachment: gigundo-rebase

Comment 45

[Brendan Eich [:brendan]]

Attachment: not-so-gigundo-rebase

Hope to have some travel downtime this week to do tests and cleanup work...

/be

Comment 46

[Brendan Eich [:brendan]]

Attachment: stupendo-rebase

Comment 47

[Brendan Eich [:brendan]]

Attachment: mini-rebase on top of stupendo-rebase

Just jsbool.cpp, applied with patch --fuzz=4 but here it is for you loyal testers (you are there, aren't you?).

/be

Comment 48

[Till Schneidereit [:till]]

> stupendo-rebase

I'm curious: what is the reason for not getting this reviewed and into the tree, disabling it in everything but Nightly?

Comment 49

[David Bruant]

(In reply to Till Schneidereit [:till] from comment #48)
> > stupendo-rebase
> 
> I'm curious: what is the reason for not getting this reviewed and into the
> tree, disabling it in everything but Nightly?
I love this idea! I know that, personally, I would be more likely to play with it if it lands on Nightly. It's probably the case for most JS devs.
I would write the doc and devs could be encouraged to check it out even more if they have doc.

Comment 50

[Till Schneidereit [:till]]

So, there's this discussion about landing new web-facing features going on over on dev-platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/ihjMCbOM3qA

I don't know enough about the state of discussion about value objects to know that landing them would be in accordance with the policy that's converged on. It seems to me that there *is* a lot of interest in value objects, and would love to get them into the hands of developers in a restricted fashion, at least.

Also, those rebases must be painful.

Comment 51

[Brandon Benvie [:benvie]]

(In reply to Till Schneidereit [:till] from comment #50)
> So, there's this discussion about landing new web-facing features going on
> over on dev-platform:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/ihjMCbOM3qA
> 
> I don't know enough about the state of discussion about value objects to
> know that landing them would be in accordance with the policy that's
> converged on. It seems to me that there *is* a lot of interest in value
> objects, and would love to get them into the hands of developers in a
> restricted fashion, at least.
> 
> Also, those rebases must be painful.

I'm fairly sure the idea is that if something is prefed off then it's fair game to have it in nightly/beta.

I also think it would be excellent if this was landed (and hidden behind a pref).

Comment 52

[Brendan Eich [:brendan]]

Attachment: moderate rebase

I want to get into landing shape this week...

/be

Comment 53

[Tom S [:evilpie]]

Comment on attachment 771877 [details] [diff] [review]
moderate rebase

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

Sorry, this probably seems quite passive aggressive, but I started looking at this on the weekend and sadly never good into looking at the real deal. So this is just some style nits :(

::: js/public/Value.h
@@ -911,5 @@
>          data = MAGIC_TO_JSVAL_IMPL(why);
>      }
>  
>      bool setNumber(uint32_t ui) {
> -        if (ui > JSVAL_INT_MAX) {

Seems unrelated.

::: js/src/builtin/Array.js
@@ +22,5 @@
>          return -1;
>  
>      var k;
>      /* Step 7. */
> +    if (n >= 0) {

This seems unrelated.

::: js/src/builtin/Int64Object.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sw=4 et tw=99 ft=cpp:

Please use the short MPL2.0, applies to other files as well.

::: js/src/builtin/IntUtility.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Interesting Waldo should have a look at this, we try to have most of our conversion utilities in mfbt/

::: js/src/builtin/Object.cpp
@@ +962,5 @@
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, args, "Object.isValue", &v))
> +        return false;
> +
> +    vp->setBoolean(!v.isObject() || v.toObject().isValue());

This should be: args.rval().setBoolean()

@@ +971,5 @@
> +obj_isObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, args, "Object.isValue", &v))

This seems unrelated as well. Also note Object.isObject.

@@ +974,5 @@
> +    RootedValue v(cx);
> +    if (!GetFirstArgument(cx, args, "Object.isValue", &v))
> +        return false;
> +
> +    vp->setBoolean(v.isObject());

args.rval().setBoolean()

::: js/src/builtin/TestingFunctions.cpp
@@ +511,5 @@
>      }
>  
> +    JSBool b;
> +    if (!ToBoolean(cx, vp[2], &b))
> +        return JS_FALSE;

false

::: js/src/jsapi.cpp
@@ +281,5 @@
>            case 'b':
> +          {
> +            bool b;
> +            if (!ToBoolean(cx, *sp, &b))
> +                return JS_FALSE;

false. I am starting to think that fallible ToBoolean could be in an other patch.

::: js/src/jsapi.h
@@ +1171,5 @@
>  
>  /*
>   * A jsid is an identifier for a property or method of an object which is
> + * either an interned string, a 31-bit signed integer, an object (a private
> + * name object, or E4X QName object if XML is enabled), or an internal code

whooa.

::: js/src/jsbool.cpp
@@ +207,5 @@
>      JS_ASSERT(obj);
>      return obj->as<BooleanObject>().unbox();
>  }
> +
> +#define METERING 1

For landing we probably want to remove this.

Comment 54

[Brendan Eich [:brendan]]

Attachment: yet another rebase

Comment 55

[nemo]

Apologies for a non-constructive comment, but I was wondering.  Is this going to end up in nightlies as discussed in comment #49 ?
I'm wondering because:
https://wiki.mozilla.org/Javascript:SpiderMonkey:OdinMonkey 
no longer lists this bug as of:
https://wiki.mozilla.org/index.php?title=Javascript%3ASpiderMonkey%3AOdinMonkey&diff=677040&oldid=669697

Did merging this turn out to be too much trouble/risk?

Comment 56

[Luke Wagner [:luke]]

For a while the official plan to get float32 codegen in asm.js was to leverage the float32 value object.  That is still the long-term plan, but we found a more immediate solution with Math.fround (bug 900120).

Comment 57

[Niko Matsakis [:nmatsakis]]

FYI: As part of the work on SIMD (bug 904913), we'll probably wind up reimplementing/porting a good deal of this work in terms of typed objects (bug 578700).

Comment 58

[Niko Matsakis [:nmatsakis]]

Having read the patch, I see there's a lot in here! I think the only parts we'll need for the SIMD work is to implement the core idea of a JSObject* that acts like a value (presumably it'll be a TypedObject* that overrides === and typeof). I imagine we'll be cribbing quite a bit from this patch with respect to how to go about making === and typeof more overridable than they are today. The other parts of the patch -- operator overloading, parsing uint64s, etc -- should probably stay in this bug for the time being, though I guess we'll eventually want to overload the operators for SIMD as well to make things feel as natural as possible.

Comment 61

[André Bargull [:anba]]

[1] indicates that int64 is currently off the plate in favour of BigInt, there dup'ing forward to bug 1366287.

[1] https://github.com/tc39/tc39-notes/blob/master/es7/2017-01/jan-25.md#15iv-progress-report-and-request-for-comments-on-64-bit-int-support

Comment 62

[Marco Zehe (:MarcoZ)]

I see nothing that warrants an a11y review on a closed bug. Removing flag.