(Away) Assignee
	Description • 10 years ago

Currently Windows PGO is enabled per-directory with MSVC_ENABLE_PGO. With VS2013 we don't need to worry about overloading the linker and should be able to PGO everything.

	Mike Hommey [:glandium]
	Comment 1 • 10 years ago

Please do check how much build time this adds, though.

	zhoubcfan@163.com
	Comment 2 • 10 years ago

I think this need to be conditional if express version is supported.

	zhoubcfan@163.com
	Comment 3 • 10 years ago

(In reply to zhoubcfan from comment #2)
> I think this need to be conditional if express version is supported.

Please forget this, pgo is not available in express version.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 4 • 10 years ago

This bug is not about local builds. Requiring PGO, a two-step process, for all local builds would be silly.

	(Away) Assignee
	Comment 5 • 10 years ago

Removing the MSVC_ENABLE_PGO test gives me an error dialog when it tries to capture the profile:

---------------------------
shlibsign.exe - Application Error
---------------------------
The application was unable to start correctly (0xc000007b). Click OK to close the application. 
---------------------------
OK   
---------------------------

Smells a lot like bug 919735 comment 2.

	(Away) Assignee
	Comment 6 • 10 years ago

The shlibsign errors were caused by shlibsign.exe's use of nss3.dll, which was itself getting PGO'd as well as using mozglue.dll which was also getting PGO'd.

	(Away) Assignee
	Comment 7 • 10 years ago

This took my PGO build from 58 minutes to 75 minutes. Ouch - but we're still better than VS2010.

I didn't yet remove uses of MSVC_ENABLE_PGO. Want to make sure this sticks first.

	Mike Hommey [:glandium]
	Comment 8 • 10 years ago

Comment on attachment 8508393 [details] [diff] [review]
PGO all the things (except nss and mozglue)

Review of attachment 8508393 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/external/nss/moz.build
@@ +6,5 @@
>  
>  DIRS += ['crmf']
>  
> +if CONFIG['_MSC_VER']:
> +    NO_PGO = True

Please add comments as to why this is necessary. Also, please file a bug on the why it fails, I don't see why we'd actually be /running/ nss tools during the instrumentation phase of the build.

	(not currently active) Ted Mielczarek
	Comment 9 • 10 years ago

(In reply to Mike Hommey [:glandium] from comment #8)
> Please add comments as to why this is necessary. Also, please file a bug on
> the why it fails, I don't see why we'd actually be /running/ nss tools
> during the instrumentation phase of the build.

I suspect this is just fallout from running 'make stage-package' so we can run the profiling scenario from the staged bits. We could probably just skip the NSS signing during that package staging. Alternately we could maybe use a workaround like bug 919735 if that's what's broken.

	(Away) Assignee
	Comment 10 • 10 years ago

> Alternately we could maybe use a workaround like bug 919735 if that's what's broken.

I was typing up a comment like "but I feel dirty about that fix and hesitate to duplicate it" until I realized that the path hack is already in packager.py. We just need to move it a few lines to a more general location. I don't mind that so much :)

	(not currently active) Ted Mielczarek
	Comment 12 • 10 years ago

Comment on attachment 8510195 [details] [diff] [review]
Part 1: Move the path hack so that shlibsign uses it

Review of attachment 8510195 [details] [diff] [review]:
-----------------------------------------------------------------

Well that's not so bad then, is it?

	(Away) Assignee
	Comment 13 • 10 years ago

With Part 1, we no longer need to opt-out any files. Stripping this patch down to just the config.mk piece.

	(Away) Assignee
	Comment 14 • 10 years ago

Fixed an indentation error caught by 64-bit try.

	(Away) Assignee
	Comment 15 • 10 years ago

On 32-bit I failed xpcshell at test_crash_purevirtual.js because the compiler completely removed nsTestCrasher.cpp's PureVirtualCall()!

To get around this I marked testcrasher.dll as INTERNAL_TOOLS (which it is) since that prevents PGO.

	(Away) Assignee
	Comment 16 • 10 years ago

On Win64 I still failed to build (even with the indentation fix): https://tbpl.mozilla.org/?tree=Try&rev=7e9c4d2ca46c

c:\builds\moz2_slave\try-w64-0000000000000000000000\build\dom\mobileconnection\mobileconnection.cpp(727) : fatal error C1001: An internal error has occurred in the compiler.
mozmake.exe[6]: *** [xul.dll] Error 1
mozmake.exe[6]: *** Deleting file 'xul.dll'
mozmake.exe[5]: *** [toolkit/library/target] Error 2
mozmake.exe[4]: *** [compile] Error 2
mozmake.exe[3]: *** [default] Error 2
mozmake.exe[2]: *** [realbuild] Error 2
mozmake.exe[1]: *** [profiledbuild] Error 2
mozmake.exe: *** [build] Error 2 

I haven't yet investigated, but this sounds bad. This is the kind of error that we used to see in 32-bit builds before Update 3. I wouldn't be surprised if the Win64 PGO were less mature and not yet ready for the full weight of libxul.

	(not currently active) Ted Mielczarek
	Comment 17 • 10 years ago

Comment on attachment 8511746 [details] [diff] [review]
Part 0: Mark testcrasher.dll as INTERNAL_TOOLS to prevent PGO

Review of attachment 8511746 [details] [diff] [review]:
-----------------------------------------------------------------

Hah!

	(not currently active) Ted Mielczarek
	Comment 18 • 10 years ago

(In reply to David Major [:dmajor] (UTC+13) from comment #16)
> I haven't yet investigated, but this sounds bad. This is the kind of error
> that we used to see in 32-bit builds before Update 3. I wouldn't be
> surprised if the Win64 PGO were less mature and not yet ready for the full
> weight of libxul.

In general we've worked around bugs in the PGO compiler for the entirety of the time we've been using PGO, so this doesn't surprise me. Our standard operating procedure here is to disable PGO for the source file that's causing the issue and try to file a ticket with Microsoft.

	(Away) Assignee
	Comment 19 • 10 years ago

> In general we've worked around bugs in the PGO compiler for the entirety of
> the time we've been using PGO, so this doesn't surprise me. Our standard
> operating procedure here is to disable PGO for the source file that's
> causing the issue and try to file a ticket with Microsoft.

For the 32-bit failures before Update 3, it was a matter of volume, not specific files: if you disabled a single file, you'd just crash on the next one. I suspect the Win64 crash is in the same boat, but I haven't tested this theory yet. I'll look more when I get back.

	(Away) Assignee
	Comment 20 • 10 years ago

Well I'm quite happy to be wrong about this. MobileConnection.cpp was indeed a one-off failure. We can just disable PGO for that file on Win64, and I'll send a LINK_REPRO to Microsoft.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f7591a7e04e4

	Mike Hommey [:glandium]
	Comment 22 • 10 years ago

Comment on attachment 8515857 [details] [diff] [review]
Disable PGO for MobileConnection.cpp

Review of attachment 8515857 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobileconnection/moz.build
@@ +50,5 @@
> +    'MobileConnection.cpp', # Non-unified due to no_pgo
> +]
> +
> +if CONFIG['_MSC_VER'] and CONFIG['CPU_ARCH'] == 'x86_64':
> +    SOURCES['MobileConnection.cpp'].no_pgo = True # VS2013 crashes, bug 1084162

maybe make that a version-specific thing?

	(Away) Assignee
	Comment 23 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/574a85c4faf9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/115e2868a577
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/718c16f55901
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2a43a51c82

Didn't bother with a version check since PGO implicitly needs >= 2013 nowadays.

	Mike Hommey [:glandium]
	Comment 24 • 10 years ago

(In reply to David Major [:dmajor] (UTC+13) from comment #23)
> Didn't bother with a version check since PGO implicitly needs >= 2013
> nowadays.

The point was that PGO would have been enabled for 2014.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/574a85c4faf9
https://hg.mozilla.org/mozilla-central/rev/115e2868a577
https://hg.mozilla.org/mozilla-central/rev/718c16f55901
https://hg.mozilla.org/mozilla-central/rev/1f2a43a51c82

	(Away) Assignee
	Comment 26 • 10 years ago

Talos email shows some modest wins (2-5%) from this.

I filed https://connect.microsoft.com/VisualStudio/feedback/details/1020301 about the crash in MobileConnection.cpp.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 27 • 10 years ago

we have 2 slight regressions on windows in dromaeo DOM on win7 and win8.  I see this on Aurora while investigating the issues seen after the uplift.

	Jan de Mooij [:jandem]
	Comment 28 • 9 years ago

Shouldn't we also remove all these |MSVC_ENABLE_PGO = True| lines from moz.build files?

I just spent a lot of time investigating some PGO performance issues, then I realized the MSVC_ENABLE_PGO moz.build flag has no effect :( (Or maybe I'm missing something subtle...)

	(Away) Assignee
	Comment 29 • 9 years ago

Thanks for the reminder, Jan. I'll file a follow-up bug. Sorry for the trouble.

(In reply to David Major [:dmajor] from comment #7)
> I didn't yet remove uses of MSVC_ENABLE_PGO. Want to make sure this sticks first.
I'm going to say nearly-six-months counts as stuck!

	Jan de Mooij [:jandem]
	Comment 30 • 9 years ago

Thanks! Sorry, I didn't see comment 7 :)