Closed Bug 597814 Opened 9 years ago Closed 9 years ago

Optimize js_DoubleToECMAInt32 for MSVC and gcc x86_64

Categories

(Core :: JavaScript Engine, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

(Whiteboard: [fixed-in-tracemonkey][suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib])

Attachments

(1 file)

GCC x86 has optimized path for js_DoubleToECMAInt32.  When turned on for MSVC too, we can get win for math-cordic on Windows (Core 2 Dou 2GHz).

  math:                1.60x as fast      71.6ms +/- 7.9%    44.8ms +/- 5.4%     significant
    cordic:            2.88x as fast      36.3ms +/- 1.6%    12.6ms +/- 7.7%     significant
    partial-sums:      -                  24.2ms +/- 21.7%    20.3ms +/- 4.1% 
    spectral-norm:     ??                 11.1ms +/- 10.7%    11.9ms +/- 9.6%     not conclusive: might be *1.072x as slow*

binaries is http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/m_kato@ga2.so-net.ne.jp-5b0ca5e2bb63/
What JIT(s) are running with that? (Asking because ideally, js_DoubleToECMAInt32 shouldn't be hot in SunSpider.)
(In reply to comment #1)
> What JIT(s) are running with that? (Asking because ideally,
> js_DoubleToECMAInt32 shouldn't be hot in SunSpider.)

JM+TM.
On gcc x86_64 (Althon64 X2 2.2GHz)

  math:                1.23x as fast      52.5ms +/- 2.2%    42.7ms +/- 1.1%     significant
    cordic:            1.81x as fast      21.3ms +/- 3.9%    11.8ms +/- 2.6%     significant
    partial-sums:      -                  14.5ms +/- 2.6%    14.2ms +/- 2.1% 
    spectral-norm:     -                  16.7ms +/- 2.1%    16.7ms +/- 2.1%
Do we need to figure out why this is being called at all, per comment 1?  25ms off the Windows sunspider score sounds ... desirable.  ;)
blocking2.0: --- → ?
(In reply to comment #4)
> Do we need to figure out why this is being called at all, per comment 1?  25ms
> off the Windows sunspider score sounds ... desirable.  ;)

This data is on gcc x86_64.

JM only : 0 calls
TM only : 299,989 calls
JM+TM   : 299,906 calls
no JIT  : 300,000 calls

So this function is critical for JM+TM or TM.  "1.1 << 2" uses this function on not JM.
Sounds like we should optimize this in TM. Dvander, what does the code look like you use?
Thanks, m_kato!

/be
I think the JM code for this is http://hg.mozilla.org/mozilla-central/file/b439e73f33b7/js/src/methodjit/FastOps.cpp#l324 and following...  Note the various special cases with constant-folding there, etc...

That said, the math-cordic use is in cordicsincos and in that function the values are non-constant... but also >> is applied to things that should be statically provable to be integers.  Does JM just manage to make them integers while TM fails to?
Note that if JM failed to do that, then lhs->isNotType(JSVAL_TYPE_INT32) would test true in that compiler function and we'd land in stubs::Ursh or stubs::Rsh which call ValueToECMAUint32 and ValueToECMAInt32 respectively, which call js_DoubleToECMA(U)Int32.

So I bet the type inference is the difference in TM vs JM here.
Er, one minor correction.  The LHS of ">>" in cordic is only provably an integer assuming no overflow on addition or subtraction.
JM fast-paths the double->int conversion using SSE, guarding against the hard cases that DoubleToEcmaUint32 handles. It's handled by the macro assembler, see branchTruncateDoubleToInt32()
But that only happens if rhs->isConstant(), no?
blocking2.0: ? → -
so, can we take this patch or what? I'm confused. Or do we wait until it's fixed in the tracer. It's a big win on SS--we could just take it, since there will be cases where this function does get used.
status2.0: --- → wanted
http://hg.mozilla.org/tracemonkey/rev/b7869eca1558

Let's do a followup for the tracer bug
Whiteboard: [fixed-in-tracemonkey]
Bug 600016 seems to be about a similar tracer issue... is it about the same one?
http://hg.mozilla.org/mozilla-central/rev/b7869eca1558
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Did that followup ever get filed?
Duplicate of this bug: 585695
Whiteboard: [fixed-in-tracemonkey] → [fixed-in-tracemonkey][suspect-regress-ts_cold_generated_med]
A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :(

  Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386
  New     : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5
  Change  : +10.475 (2.49% / z=2.847)
  Graph   : http://mzl.la/bZFaB3

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
There is no way on earth this changeset is responsible for the cold start change. The bot must be on crack.
Whiteboard: [fixed-in-tracemonkey][suspect-regress-ts_cold_generated_med] → [fixed-in-tracemonkey][suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css]
A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386
  New     : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5
  Change  : -112.809 (-5.6% / z=2.787)
  Graph   : http://mzl.la/9gFu4n

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
Whiteboard: [fixed-in-tracemonkey][suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css] → [fixed-in-tracemonkey][suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib]
A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386
  New     : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5
  Change  : -11.226 (-8.8% / z=2.659)
  Graph   : http://mzl.la/bZu5UP

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386
  New     : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5
  Change  : -109.155 (-5.34% / z=2.197)
  Graph   : http://mzl.la/b0dlou

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386
  New     : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5
  Change  : -11.749 (-9.06% / z=2.866)
  Graph   : http://mzl.la/avZij4

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.
> The bot must be on crack.

The bot just knows that it has a regression across a time range that includes the push that pushed this changeset (and all sorts of other TM stuff) to m-c....
Assignee: general → m_kato
I filed bug 601670 to follow up on the TM issue here.
You need to log in before you can comment on or make changes to this bug.