Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Stylus tests failing in Node-ChakraCore #498

Open
digitalinfinity opened this issue Mar 13, 2018 · 6 comments
Open

Stylus tests failing in Node-ChakraCore #498

digitalinfinity opened this issue Mar 13, 2018 · 6 comments
Assignees

Comments

@digitalinfinity
Copy link
Contributor

  • Version: 10.0.0-nightly
  • Platform: all
  • Subsystem: chakracore

Repro steps (attempted on Linux):

Result: 26 pass, 1 fail

  1) integration
       bifs floats:

      AssertionError: expected 'body {\n  foo: 0rem;\n  foo: 0.1rem;\n  foo: 0.2rem;\n  foo: 0.3rem;\n  foo: 0.4rem;\n  foo: 0.5rem;\n  foo: 0.6rem;\n  foo: 0.7rem;\n  foo: 0.8rem;\n  foo: 0.9rem;\n  foo: 1rem;\n  foo: 1.1rem;\n  foo: 1.2rem;\n  foo: 1.3rem;\n  foo: 1.4rem;\n  foo: 1.5rem;\n  foo: 1.6rem;\n  foo: 1.7rem;\n  foo: 1.8rem;\n  foo: 1.9rem;\n  foo: 2rem;\n  foo: 50%;\n  foo: 33.333333333333336%;\n  foo: 25%;\n  foo: 20%;\n  foo: 16.666666666666668%;\n  foo: 14.285714285714286%;\n  foo: 12.5%;\n  foo: 11.11111111111111%;\n  foo: 10%;\n  foo: 9.090909090909091%;\n  foo: 8.333333333333334%;\n  foo: 1;\n  foo: 1;\n  foo: 1;\n}' to be 'body {\n  foo: 0rem;\n  foo: 0.1rem;\n  foo: 0.2rem;\n  foo: 0.3rem;\n  foo: 0.4rem;\n  foo: 0.5rem;\n  foo: 0.6rem;\n  foo: 0.7rem;\n  foo: 0.8rem;\n  foo: 0.9rem;\n  foo: 1rem;\n  foo: 1.1rem;\n  foo: 1.2rem;\n  foo: 1.3rem;\n  foo: 1.4rem;\n  foo: 1.5rem;\n  foo: 1.6rem;\n  foo: 1.7rem;\n  foo: 1.8rem;\n  foo: 1.9rem;\n  foo: 2rem;\n  foo: 50%;\n  foo: 33.333333333333336%;\n  foo: 25%;\n  foo: 20%;\n  foo: 16.666666666666668%;\n  foo: 14.285714285714286%;\n  foo: 12.5%;\n  foo: 11.11111111111111%;\n  foo: 10%;\n  foo: 9.090909090909092%;\n  foo: 8.333333333333334%;\n  foo: 1;\n  foo: 1;\n  foo: 1;\n}'
      + expected - actual

         foo: 14.285714285714286%;
         foo: 12.5%;
         foo: 11.11111111111111%;
         foo: 10%;
      -  foo: 9.090909090909091%;
      +  foo: 9.090909090909092%;
         foo: 8.333333333333334%;
         foo: 1;
         foo: 1;
         foo: 1;
@MSLaguana
Copy link
Contributor

I believe this is the same as the chakracore issue chakra-core/ChakraCore#2512

@IrinaYatsenko
Copy link

IrinaYatsenko commented Mar 22, 2018

Isolated repro:
(100.0/11).toString(10);

They expect output: 9.090909090909092
We output: 9.090909090909091

Debugger shows all three numbers as 9.0909090909090917165
a2e8ba2f 40222e8b
00101111 10111010 11101000 10100010 2f ba e8 a2
10001011 00101110 00100010 01000000 8b 2e 22 40

Note: we have separate code path for decimal conversion.

@IrinaYatsenko
Copy link

IrinaYatsenko commented Mar 26, 2018

FDblToRgbPrecise produces 9.090909090909092 representation.
FDblToRgbFast (which we end up using) produces 9.090909090909091 representation.

FDblToRgbFast hasn't changed since 2006 (other than handling errors and moving helpers into namespace).

Hiding FDblToRgbPrecise behind FDblToRgbFast goes as far back as 2002 (without any interesting comments on why it's this way).

Did a mini-benchmark: in x86 Release build FDblToRgbPrecise is about 2.8 times slower than FDblToRgbFast (a loop over 10^6 doubles: 733949944 & 263994008 nanoseconds).

@IrinaYatsenko
Copy link

Replacing FDblToRgbFast with FDblToRgbPrecise regressed the following benchmarks:
Kraken: Json-stringify-tinderbox (19%)
Octane: Splay (14%), Splay latency (6%)
SunSpider: String-tagcloud (5-17%)

Discussed with Louis and he thinks regressing perf of JSON.stringify like this isn't acceptable: JSON.stringify({ 'pi': 3.14 }) calls ToString and it's not inconceivable to have real life scenarios with json describing objects with many floating point properties.

@digitalinfinity
Copy link
Contributor Author

JSON I can see- why does splay regress with that change? I thought Splay was memory intensive. Is the String-tagcloud case also using JSON stringify in the hot path?

@IrinaYatsenko
Copy link

IrinaYatsenko commented Mar 28, 2018

Splay test generates a new random number and then converts it to string in a loop (see InsertNewNode()). Ends up calling ToString 10K+ times.

String-tagcloud creates "a href" tag using popularity numbres, which are converted to string. Calls ToString 2.5K+ times.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants