918350 - On the subject of floating-point constants.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

The following is a series of floating-point constant patches.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: jsnum-jsinfinity.patch

Comment 2

[Dan Gohman [:sunfish]]

Attachment: jsnum-jsnan.patch

Comment 3

[Dan Gohman [:sunfish]]

Attachment: jsnum-jsruntime-init.patch

Comment 4

[Dan Gohman [:sunfish]]

Attachment: jsnum-allones-nan.patch

Comment 5

[Dan Gohman [:sunfish]]

Attachment: jsnum-cleanup-constants.patch

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 807219 [details] [diff] [review]
jsnum-jsnan.patch

This adds a new mozilla::GenericNaN() function, which returns the value that js_NaN was initialized to.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 807220 [details] [diff] [review]
jsnum-jsruntime-init.patch

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

::: js/src/jsnum.cpp
@@ +46,5 @@
>  
>  using namespace js;
>  using namespace js::types;
>  
> +using mozilla::BitwiseCast;

I don't see new BitwiseCast uses, is this needed?

@@ +1151,5 @@
>       * Our NaN must be one particular canonical value, because we rely on NaN
>       * encoding for our value representation.  See Value.h.
>       */
>      d = GenericNaN();
>      number_constants[NC_NaN].dval = d;

Dunno if subsequent patches clean this up or not, but you might as well get rid of the |d| temporary for these while you're doing this.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 807221 [details] [diff] [review]
jsnum-allones-nan.patch

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

Not that I disbelieve you, but I'd be pretty skeptical of this assertion without justification if I ran across it in comments.  Anticipate my inner cynic!

::: mfbt/FloatingPoint.h
@@ +203,5 @@
>  UnspecifiedNaN()
>  {
> +  /*
> +   * If we can use any quiet NaN, we might as well use the all-ones NaN,
> +   * since it's cheap to materialize on common platforms.

Link/citation for this assertion?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 807219 [details] [diff] [review]
jsnum-jsnan.patch

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

I'd be surprised if all of these places strictly must use this particular NaN pattern, but it's not worth the trouble to audit, particularly before there's a setNaN() to get rid of the obvious ones.

I won't be entirely surprised if something breaks because UnspecifiedNaN's value changed, if the new value isn't JS-safe.  I don't remember what makes a value JS-safe, offhand, unfortunately.  (I once tried to get some comments added to describe the restrictions, then things got sidetracked into an argument over the semantics of "canonical" [there are multiple safe patterns for the JS engine, whereas canonical suggests just one] and I never finished it up.  :-\ )

::: js/src/jsmath.cpp
@@ +160,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
>      if (args.length() == 0) {
> +        args.rval().setDouble(GenericNaN());

There's an awful lot of this setDouble(nan) stuff in this patch.  Rather than require people to know what NaN to pass in, we should add JS::Value::setNaN() and convert everyone over to that.  Followup patch?  Or fold it in here, whichever's easier for you.

::: mfbt/FloatingPoint.h
@@ +189,5 @@
> + * Among other properties, this NaN's bit pattern conforms to JS::Value's
> + * bit pattern restrictions.
> + */
> +static MOZ_ALWAYS_INLINE double
> +GenericNaN()

This isn't the right header for this -- it's a JS-targeted thing, so it should be in a JS header.  Let's put JS::GenericNaN in js/public/Value.h, seeing as the particular pattern only matters in JS because of its flowing into values.

Comment 10

[Dan Gohman [:sunfish]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> ::: js/src/jsnum.cpp
> @@ +46,5 @@
> >  
> >  using namespace js;
> >  using namespace js::types;
> >  
> > +using mozilla::BitwiseCast;
> 
> I don't see new BitwiseCast uses, is this needed?

Nope; removed.

> @@ +1151,5 @@
> >       * Our NaN must be one particular canonical value, because we rely on NaN
> >       * encoding for our value representation.  See Value.h.
> >       */
> >      d = GenericNaN();
> >      number_constants[NC_NaN].dval = d;
> 
> Dunno if subsequent patches clean this up or not, but you might as well get
> rid of the |d| temporary for these while you're doing this.

Done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> ::: mfbt/FloatingPoint.h
> @@ +203,5 @@
> >  UnspecifiedNaN()
> >  {
> > +  /*
> > +   * If we can use any quiet NaN, we might as well use the all-ones NaN,
> > +   * since it's cheap to materialize on common platforms.
> 
> Link/citation for this assertion?

I added a comment.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> ::: js/src/jsmath.cpp
> @@ +160,5 @@
> >  {
> >      CallArgs args = CallArgsFromVp(argc, vp);
> >  
> >      if (args.length() == 0) {
> > +        args.rval().setDouble(GenericNaN());
> 
> There's an awful lot of this setDouble(nan) stuff in this patch.  Rather
> than require people to know what NaN to pass in, we should add
> JS::Value::setNaN() and convert everyone over to that.  Followup patch?  Or
> fold it in here, whichever's easier for you.

Folded in here.

> ::: mfbt/FloatingPoint.h
> @@ +189,5 @@
> > + * Among other properties, this NaN's bit pattern conforms to JS::Value's
> > + * bit pattern restrictions.
> > + */
> > +static MOZ_ALWAYS_INLINE double
> > +GenericNaN()
> 
> This isn't the right header for this -- it's a JS-targeted thing, so it
> should be in a JS header.  Let's put JS::GenericNaN in js/public/Value.h,
> seeing as the particular pattern only matters in JS because of its flowing
> into values.

Done.

All patches checked in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/77a16602b99c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e727132b19c
https://hg.mozilla.org/integration/mozilla-inbound/rev/141c61d174ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0888eb399ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/5953301b310c

If any of my corrections are unsatisfactory, please let me know and I'll correct them :).

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77a16602b99c
https://hg.mozilla.org/mozilla-central/rev/9e727132b19c
https://hg.mozilla.org/mozilla-central/rev/141c61d174ee
https://hg.mozilla.org/mozilla-central/rev/d0888eb399ec
https://hg.mozilla.org/mozilla-central/rev/5953301b310c
https://hg.mozilla.org/mozilla-central/rev/1ff9afcf5bec
https://hg.mozilla.org/mozilla-central/rev/e309a5ebdd9b

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Dan Gohman [:sunfish] from comment #10)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> > ::: mfbt/FloatingPoint.h
> > @@ +189,5 @@
> > > + * Among other properties, this NaN's bit pattern conforms to JS::Value's
> > > + * bit pattern restrictions.
> > > + */
> > > +static MOZ_ALWAYS_INLINE double
> > > +GenericNaN()
> > 
> > This isn't the right header for this -- it's a JS-targeted thing, so it
> > should be in a JS header.  Let's put JS::GenericNaN in js/public/Value.h,
> > seeing as the particular pattern only matters in JS because of its flowing
> > into values.
> 
> Done.

But the commit message still mentioned mozilla::GenericNaN(). Not worth changing it now, but please try to remember commit messages in the future ;)