nodejs / node-chakracore

Node.js on ChakraCore :sparkles::turtle::rocket::sparkles:
Other
1.92k stars 340 forks source link

Stylus tests failing in Node-ChakraCore #498

Open digitalinfinity opened 6 years ago

digitalinfinity commented 6 years ago

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

I believe this is the same as the chakracore issue https://github.com/Microsoft/ChakraCore/issues/2512

IrinaYatsenko commented 6 years ago

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

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

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

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

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.