Closed Bug 1056529 Opened 6 years ago Closed 6 years ago

[BaselineCompiler] Sunspider-math-cordic takes 53% more time on windows

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: h4writer, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When running ss-math-cordic with --no-ion I get the following difference on Windows/Linux:

Windows: 47.1ms
Linux: 30.6ms

I found already what the issue can be:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#2506
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#3072

We disabled some optimizations for windows because of PGO.
PGO is currently disabled on the JS directory. Shouldn't we have removed all those flags at the same time. Since they disable all optimizations on those functions and it hits us.

Just grepping for _MSC_VER and PGO shows a lot of places we can remove this.
Blocks: 1028242
Flags: needinfo?(jdemooij)
E.g. on Raytrace we spend 6% in epowi on windows, while we only spend 3% in linux
http://dxr.mozilla.org/mozilla-central/source/js/src/jsmath.cpp#636

Removing this, gives us the same amount of time spend in epowi on windows.
Note: this only improve our score marginally on raytrace. But it could help other benchmarks much more.
(In reply to Hannes Verschore [:h4writer] from comment #1)
> PGO is currently disabled on the JS directory. Shouldn't we have removed all
> those flags at the same time. Since they disable all optimizations on those
> functions and it hits us.

PGO is only disabled for MSVC 2010. Although I doubt it, it's possible newer versions still have some of these bugs. We could disable the workarounds for MSVC 2010 though, and file a followup bug to recheck once we update the compiler...

How much does math-cordic improve if you remove this?
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #3)
> How much does math-cordic improve if you remove this?

We get the same speed as linux in that case. So windows improves from 47.1ms to +-30.6ms.
(In reply to Hannes Verschore [:h4writer] from comment #4)
> We get the same speed as linux in that case. So windows improves from 47.1ms
> to +-30.6ms.

Wow, good find!

Btw, if fallback stub performance matters so much on this test, is it possible we're not attaching a stub for some operation?
(In reply to Hannes Verschore [:h4writer] from comment #2)
> E.g. on Raytrace we spend 6% in epowi on windows, while we only spend 3% in
> linux
> http://dxr.mozilla.org/mozilla-central/source/js/src/jsmath.cpp#636
> 
> Removing this, gives us the same amount of time spend in epowi on windows.
> Note: this only improve our score marginally on raytrace. But it could help
> other benchmarks much more.

I measured this again. And I saw an improvement of 5% - 7% when removing that.
Attached patch PatchSplinter Review
This patch removes all pragmas we added to work around PGO bugs. If we run into PGO bugs with MSVC 2013 we can just reverse this patch, but from what I've seen MSVC 2013 PGO is a lot more mature.

Windows Try push: https://tbpl.mozilla.org/?tree=Try&rev=e714d021ce82

Windows PGO Try push: https://tbpl.mozilla.org/?tree=Try&rev=6ac9d6bbadad
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8479292 - Flags: review?(hv1989)
Attachment #8479292 - Flags: review?(hv1989) → review+
(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Hannes Verschore [:h4writer] from comment #4)
> > We get the same speed as linux in that case. So windows improves from 47.1ms
> > to +-30.6ms.
> 
> Btw, if fallback stub performance matters so much on this test, is it
> possible we're not attaching a stub for some operation?

Will you open a follow-up bug for this?
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a10e38f42d

(In reply to Hannes Verschore [:h4writer] from comment #8)
> Will you open a follow-up bug for this?

Yes, will file in a bit.
https://hg.mozilla.org/mozilla-central/rev/a2a10e38f42d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.