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

RESOLVED DUPLICATE of bug 1366287

Status

()

defect
RESOLVED DUPLICATE of bug 1366287
7 years ago
a year ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

(Blocks 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 33 obsolete attachments)

174.04 KB, patch
Details | Diff | Splinter Review
Assignee

Description

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

/be
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
Assignee

Comment 1

7 years ago
Doing only int64 and uint64 to start.

/be
Blocks: es6
Assignee

Updated

7 years ago
Assignee: general → brendan

Updated

7 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

7 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

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

/be
Assignee

Comment 7

7 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

7 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 10

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

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

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

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

7 years ago
Summary: prototype value objects → prototype int64/uint64 value objects, with operators and literal syntax
Assignee

Comment 14

7 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

7 years ago
More TODO notes:

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

/be
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

7 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

7 years ago
Blocks: 757256
Assignee

Comment 19

7 years ago
Attachment #621528 - Attachment is obsolete: true
Assignee

Comment 20

7 years ago
Rebased (painfully).

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

Comment 21

7 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #645920 - Attachment is obsolete: true
Assignee

Comment 22

7 years ago
Posted patch rebased yet again (obsolete) — Splinter Review
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

7 years ago
Attachment #653618 - Attachment is obsolete: true

Comment 24

7 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

7 years ago
Thanks, also looking forward. Anyone on WebKit working on this?
Assignee

Comment 26

7 years ago
Posted patch rebased, sorry for the delay (obsolete) — Splinter Review
Attachment #655055 - Attachment is obsolete: true
Assignee

Comment 27

7 years ago
Attachment #672544 - Attachment is obsolete: true
Assignee

Comment 29

7 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #680269 - Attachment is obsolete: true
Assignee

Comment 30

7 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #684917 - Attachment is obsolete: true
Assignee

Comment 31

7 years ago
Posted patch rebased for 2013 (obsolete) — Splinter Review
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

6 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #697075 - Attachment is obsolete: true
Assignee

Comment 33

6 years ago
Posted patch heckuva rebase (obsolete) — Splinter Review
Attachment #704290 - Attachment is obsolete: true
Assignee

Comment 34

6 years ago
Posted patch rebased yet again (obsolete) — Splinter Review
Attachment #706847 - Attachment is obsolete: true
Assignee

Comment 35

6 years ago
Posted patch big-ass rebase (obsolete) — Splinter Review
Attachment #707648 - Attachment is obsolete: true
Assignee

Comment 36

6 years ago
Posted patch copy/paste fix to last rev (obsolete) — Splinter Review
Attachment #725038 - Attachment is obsolete: true
Assignee

Comment 37

6 years ago
Attachment #725254 - Attachment is obsolete: true
Attachment #726861 - Attachment is patch: true

Comment 38

6 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

6 years ago
Posted patch rebase (obsolete) — Splinter Review
Attachment #726861 - Attachment is obsolete: true
Assignee

Comment 40

6 years ago
Posted patch rebase-o-rama (obsolete) — Splinter Review
Attachment #730591 - Attachment is obsolete: true
Assignee

Comment 41

6 years ago
Posted patch rebased (obsolete) — Splinter Review
Assignee

Updated

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

Updated

6 years ago
Attachment #733958 - Attachment is obsolete: true
Assignee

Comment 42

6 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #743456 - Attachment is obsolete: true
Assignee

Comment 43

6 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #745589 - Attachment is obsolete: true
Assignee

Comment 44

6 years ago
Posted patch gigundo-rebase (obsolete) — Splinter Review
Attachment #747672 - Attachment is obsolete: true
Assignee

Comment 45

6 years ago
Posted patch not-so-gigundo-rebase (obsolete) — Splinter Review
Hope to have some travel downtime this week to do tests and cleanup work...

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

Comment 46

6 years ago
Posted patch stupendo-rebase (obsolete) — Splinter Review
Attachment #755630 - Attachment is obsolete: true
Assignee

Comment 47

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

6 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

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

Updated

6 years ago
Blocks: JSIL
Assignee

Comment 52

6 years ago
Posted patch moderate rebase (obsolete) — Splinter Review
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

6 years ago
Attachment #771877 - Attachment is obsolete: true

Comment 55

6 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?
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: es7
No longer blocks: es6

Updated

5 years ago
Target Milestone: mozilla15 → ---
[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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: js-bigint

Updated

a year ago
Flags: sec-bounty?
Flags: in-testsuite?
Flags: in-qa-testsuite?
Flags: behind-pref-
Flags: a11y-review?
I see nothing that warrants an a11y review on a closed bug. Removing flag.
Flags: a11y-review?
Flags: sec-bounty?
Flags: in-testsuite?
Flags: in-qa-testsuite?
Flags: behind-pref-
You need to log in before you can comment on or make changes to this bug.