The default bug view has changed. See this FAQ.

Enable PGO for libmozjs on Windows

RESOLVED WORKSFORME

Status

()

Core
Build Config
RESOLVED WORKSFORME
6 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Matheus Kerschbaum)

Tracking

Trunk
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
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.
I still think we should wait until we switch compilers.  https://bugzilla.mozilla.org/show_bug.cgi?id=605033#c46
(Assignee)

Updated

6 years ago
Assignee: nobody → matjk7
Status: NEW → ASSIGNED
Depends on: 563318
OS: Mac OS X → Windows 7
Hardware: x86 → All
Has something changed so that we want to do this now? It's caused a lot of problems in the past.
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.
(Assignee)

Comment 5

5 years ago
Created attachment 575011 [details] [diff] [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?).

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).
Attachment #575011 - Flags: review?(khuey)
(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?
Depends on: 721284
Attachment #575011 - Attachment is obsolete: true
Attachment #575011 - Flags: review?(khuey)
We started doing this at some point.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
It looks like this was bug 721284, FWIW.
You need to log in before you can comment on or make changes to this bug.