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

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
ASSIGNED
5 years ago
2 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

(Blocks: 4 bugs, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 33 obsolete attachments)

174.04 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Patch coming along, hope to post it later today.

/be
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Doing only int64 and uint64 to start.

/be
Blocks: 694100
(Assignee)

Updated

5 years ago
Assignee: general → brendan

Updated

5 years ago
Keywords: dev-doc-needed
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
(Assignee)

Comment 3

5 years ago
(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
> > - 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
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
(Assignee)

Comment 6

5 years ago
Created attachment 620929 [details] [diff] [review]
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
(Assignee)

Comment 7

5 years ago
> 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
(Assignee)

Comment 8

5 years ago
(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
(Assignee)

Comment 9

5 years ago
Created attachment 621225 [details] [diff] [review]
work in progress, more binary ops done in interpreter (bitwise, ===/!==, unary still todo)
Attachment #620929 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 621333 [details] [diff] [review]
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
Attachment #621225 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 621526 [details] [diff] [review]
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
Attachment #621333 - Attachment is obsolete: true
Attachment #621526 - Flags: feedback?(luke)
(Assignee)

Comment 12

5 years ago
Created attachment 621527 [details] [diff] [review]
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
Attachment #621526 - Attachment is obsolete: true
Attachment #621526 - Flags: feedback?(luke)
Attachment #621527 - Flags: feedback?(luke)
(Assignee)

Comment 13

5 years ago
Created attachment 621528 [details] [diff] [review]
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
Attachment #621527 - Attachment is obsolete: true
Attachment #621527 - Flags: feedback?(luke)
Attachment #621528 - Flags: feedback?(luke)
(Assignee)

Updated

5 years ago
Summary: prototype value objects → prototype int64/uint64 value objects, with operators and literal syntax
Blocks: 752441
(Assignee)

Comment 14

5 years ago
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
(Assignee)

Comment 15

5 years ago
More TODO notes:

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

/be

Comment 16

5 years ago
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.
Attachment #621528 - Flags: feedback?(luke) → feedback+
(Assignee)

Comment 17

5 years ago
More TODO:

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

/be
Duplicate of this bug: 757256
(Assignee)

Updated

5 years ago
Blocks: 757256
(Assignee)

Comment 19

5 years ago
Created attachment 636289 [details] [diff] [review]
WIP, add UL suffix, LxUL and ULxL binary operators
Attachment #621528 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 645920 [details] [diff] [review]
WIP, add UL suffix, LxUL and ULxL binary operators

Rebased (painfully).

/be
Attachment #636289 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 647708 [details] [diff] [review]
rebased
Attachment #645920 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 653618 [details] [diff] [review]
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
Attachment #647708 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Created attachment 655055 [details] [diff] [review]
rebased, sad to see nice JSObject methods become static
Attachment #653618 - Attachment is obsolete: true

Comment 24

5 years ago
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

5 years ago
Thanks, also looking forward. Anyone on WebKit working on this?
(Assignee)

Comment 26

5 years ago
Created attachment 672544 [details] [diff] [review]
rebased, sorry for the delay
Attachment #655055 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 676871 [details] [diff] [review]
rebased, starting to look at the JITs
Attachment #672544 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Created attachment 680269 [details] [diff] [review]
rebased, looking at JITs on upcoming trip (only time I have to hack :-/)
Attachment #676871 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Created attachment 684917 [details] [diff] [review]
rebased
Attachment #680269 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Created attachment 689939 [details] [diff] [review]
rebased
Attachment #684917 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
Created attachment 697075 [details] [diff] [review]
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
Attachment #689939 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Created attachment 704290 [details] [diff] [review]
rebased
Attachment #697075 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
Created attachment 706847 [details] [diff] [review]
heckuva rebase
Attachment #704290 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Created attachment 707648 [details] [diff] [review]
rebased yet again
Attachment #706847 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Created attachment 725038 [details] [diff] [review]
big-ass rebase
Attachment #707648 - Attachment is obsolete: true
(Assignee)

Comment 36

4 years ago
Created attachment 725254 [details] [diff] [review]
copy/paste fix to last rev
Attachment #725038 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Created attachment 726861 [details] [diff] [review]
rebased past arrow functions (yay!)
Attachment #725254 - Attachment is obsolete: true
Attachment #726861 - Attachment is patch: true

Comment 38

4 years ago
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?
(Assignee)

Comment 39

4 years ago
Created attachment 730591 [details] [diff] [review]
rebase
Attachment #726861 - Attachment is obsolete: true
(Assignee)

Comment 40

4 years ago
Created attachment 733958 [details] [diff] [review]
rebase-o-rama
Attachment #730591 - Attachment is obsolete: true
(Assignee)

Comment 41

4 years ago
Created attachment 743456 [details] [diff] [review]
rebased
(Assignee)

Updated

4 years ago
Attachment #743456 - Attachment is patch: true
Attachment #743456 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Updated

4 years ago
Attachment #733958 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
Created attachment 745589 [details] [diff] [review]
rebased
Attachment #743456 - Attachment is obsolete: true
(Assignee)

Comment 43

4 years ago
Created attachment 747672 [details] [diff] [review]
rebased
Attachment #745589 - Attachment is obsolete: true
(Assignee)

Comment 44

4 years ago
Created attachment 752180 [details] [diff] [review]
gigundo-rebase
Attachment #747672 - Attachment is obsolete: true
(Assignee)

Comment 45

4 years ago
Created attachment 755630 [details] [diff] [review]
not-so-gigundo-rebase

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

/be
Attachment #752180 - Attachment is obsolete: true
(Assignee)

Comment 46

4 years ago
Created attachment 766358 [details] [diff] [review]
stupendo-rebase
Attachment #755630 - Attachment is obsolete: true
(Assignee)

Comment 47

4 years ago
Created attachment 766507 [details] [diff] [review]
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
Attachment #766358 - Attachment is obsolete: true
> 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

4 years ago
(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.
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.
(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).
(Assignee)

Updated

4 years ago
Attachment #766507 - Attachment is patch: true
Attachment #766507 - Attachment mime type: text/x-patch → text/plain

Updated

4 years ago
Blocks: 889783
(Assignee)

Comment 52

4 years ago
Created attachment 771877 [details] [diff] [review]
moderate rebase

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

/be
Attachment #766507 - Attachment is obsolete: true
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.
(Assignee)

Comment 54

4 years ago
Created attachment 772324 [details] [diff] [review]
yet another rebase
Attachment #771877 - Attachment is obsolete: true
Blocks: 898664

Comment 55

4 years ago
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

4 years ago
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).
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).
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.
Blocks: 1021376
No longer blocks: 694100

Updated

3 years ago
Target Milestone: mozilla15 → ---
You need to log in before you can comment on or make changes to this bug.