901000 - PJS: Math functions race since they use MathCache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

We consider Math functions safe currently, but they use a MathCache structure to cache results. This cache is engine-wide. We should either duplicate the cache per thread or not cache in PJS.

Comment 1

[Shu-yu Guo [:shu]]

This is what was causing the indeterminism in nbody.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

For the short-term, I'm planning to make PJS compile Math.sin etc to variants that do not access the cache in any way.

For the long-term, we have options.  Such as:

  A. Do not use the cache (as above)

  B. Make the cache per-thread.  
     (This has sub-options, such as whether to combine the cache results at the end of a PJS invocation, or throw them away, or leave them in place for future runs on that same thread.)

  C. Read from the cache but do not update it.
     (I.e. adapt the code to a multiple-readers/no-writers model when in PJS.)

Comment 3

[Nicholas Nethercote [inactive]]

IIRC, the math cache greatly sped up one of the Sunspider tests.  If that's still true then removing it would be a problem.

If we went with option B, I suspect combining caches wouldn't be worth it.

Comment 4

[Jeff Walden [:Waldo]]

I'd say don't use the cache.  The only reason for it is SunSpider stupidity.  Pages that benefit from it would be better off recognizing the cache opportunity themselves, and not trying to do a bunch of pointless extra work.  With no "legacy" "compatibility" constraints on PJS, we shouldn't preserve this wart there.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: bz901000-pAv1-uncached-variants.patch

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: bz901000-pBv1-regression-tests.patch

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 794031 [details] [diff] [review]
bz901000-pAv1-uncached-variants.patch

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

::: js/src/jsmath.cpp
@@ +144,5 @@
>  
> +double
> +js::math_acos_uncached(double x)
> +{
> +#if defined(SOLARIS) && defined(__GNUC__)

Is there a way to avoid duplicating this special case #if code?
For example, perhaps math_acos should delegate to math_acos_uncached rather than `acos`? But I guess that would grow the cache unnecessarily. Another option would be to use #define macros to eliminate the redundancy.

@@ +187,5 @@
>  
> +double
> +js::math_asin_uncached(double x)
> +{
> +#if defined(SOLARIS) && defined(__GNUC__)

As above.

@@ +382,5 @@
>  
> +double
> +js::math_exp_uncached(double x)
> +{
> +#ifdef _WIN32

As above.

@@ +472,5 @@
>  
> +double
> +js::math_log_uncached(double x)
> +{
> +#if defined(SOLARIS) && defined(__GNUC__)

As above.

@@ +968,5 @@
>  
> +double
> +js::math_log1p_uncached(double x)
> +{
> +#ifdef __APPLE__

As above.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: bz901000-pAv2-uncached-variants.patch

patch A, version 2: incorporates niko's suggestion of using preprocessor macros to abstract common code between impl and uncached variants.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 794675 [details] [diff] [review]
bz901000-pAv2-uncached-variants.patch

(inherits previous r+ from nmatsakis.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(My try build failed for some mysterious reason.  The failure was on the regression test added with this patch, so I am investigation further before attempting to land.)

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: bz901000-pBv2-regression-tests.patch

Added slow marking because this was causing jit-less tests to time out.

Inheriting r+ from previous review from nmatsakis.

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

try run:
  https://tbpl.mozilla.org/?tree=Try&rev=5942177b8f54

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

pushed to inbound: 

   https://hg.mozilla.org/integration/mozilla-inbound/rev/20d71d9471cb
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5409f0b566b1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20d71d9471cb
https://hg.mozilla.org/mozilla-central/rev/5409f0b566b1