673518 - Enable PGO for libmozjs on Windows

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[(no longer active)]

Apparently right now we don't do PGO for libmozjs on Windows.  We should experiment to see if enabling PGO for libmozjs is going to win us anything on the benchmarks.

Comment 1

[(not currently active) Ted Mielczarek]

We already did this test, khuey flipped it back on and it was a good 10% on some benchmarks, this was bug 641325. See, for example:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/1f30085628d15d41?pli=1

bug 605033 turned it off again to see if it was responsible for some JS topcrashes, but Kairo stated in that bug that it didn't have any impact, so we might as well re-enable it.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I still think we should wait until we switch compilers.  https://bugzilla.mozilla.org/show_bug.cgi?id=605033#c46

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Has something changed so that we want to do this now? It's caused a lot of problems in the past.

Comment 4

[(not currently active) Ted Mielczarek]

Note the dependency on bug 563318. We should try this again when we finally switch to VC 2010, since we seemed to be hitting a compiler bug last time.

Comment 5

[Matheus Kerschbaum]

Attachment: patch

Now that the switch to msvc2010 is on the horizon we can start discussing strategies on landing this.

In the past turning PGO for libmozjs on caused new crashes (bug 605033) but Kyle pointed out that this was fixed in msvc2010 (bug 605033 comment 46). Assuming PGO is saner in msvc2010 this switch should _not_ bring any new crashes, but unfortunately due to more aggressive inlining it will shift crash signatures quite a bit.

Considering that the switch to msvc2010 itself might bring new crashes or at least change crash signatures we probably want to hold off on landing this for some time (1 release cycle sounds sane enough?).

On the performance side of things, this bug is pretty good news: http://goo.gl/ZOKDc

A significant win on all perf tests (tp4, tp5, tspaint, tpaint, dromaeo_*, tsspider_*, ts_places_*) though a small regression on mem usage (*_memset and *_pbytes tests).

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Matheus Kerschbaum from comment #5)
> Created attachment 575011 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Now that the switch to msvc2010 is on the horizon we can start discussing
> strategies on landing this.
> 
> In the past turning PGO for libmozjs on caused new crashes (bug 605033) but
> Kyle pointed out that this was fixed in msvc2010 (bug 605033 comment 46).
> Assuming PGO is saner in msvc2010 this switch should _not_ bring any new
> crashes, but unfortunately due to more aggressive inlining it will shift
> crash signatures quite a bit.
> 
> Considering that the switch to msvc2010 itself might bring new crashes or at
> least change crash signatures we probably want to hold off on landing this
> for some time (1 release cycle sounds sane enough?).

for vs2005, should you use ifeq (1400,$(_MSC_VER)) instead?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We started doing this at some point.

Comment 8

[(not currently active) Ted Mielczarek]

It looks like this was bug 721284, FWIW.