(Away) Reporter
	Description • 10 years ago

Inevitably, VS2013 will be the cause of a few 'no_pgo' over the next couple years. I'd like to balance it out by reinstating PGO where it's no longer broken. Maybe we can be net positive!

http://dxr.mozilla.org/mozilla-central/search?q=no_pgo&case=false

	(Away) Reporter
	Comment 1 • 10 years ago

Here are the potential candidates (and also calling out some non-candidates). I've excluded non-shipping tools and non-Windows builds.

Jesup I'm not sure about the sctp ones, can you help me dig up some context on why they were added?

gfx/layers/moz.build
342 	SOURCES[src].no_pgo = True
Revisit bug 795594

	js/src/moz.build
492 	SOURCES['builtin/RegExp.cpp'].no_pgo = True # Bug 772303
494 	SOURCES['ctypes/CTypes.cpp'].no_pgo = True # Bug 810661

	media/libvpx/moz.build
35 	SOURCES[s].no_pgo = True
Need to keep this since a build tool depends on the .obj file format

	netwerk/sctp/datachannel/moz.build
39 	NO_PGO = True # Don't PGO
	netwerk/sctp/src/moz.build
92 	NO_PGO = True # Don't PGO
Not sure...

	toolkit/library/moz.build
77 	SOURCES['StaticXULComponentsStart.cpp'].no_pgo = True
Needed for section magic

 	xpcom/base/moz.build
93 	SOURCES['nsDebugImpl.cpp'].no_pgo = True
Deliberately avoid optimization for better stack

	xpcom/reflect/xptcall/md/win32/moz.build
36 	SOURCES['xptcinvoke.cpp'].no_pgo = True 
Revisit bug 950323

	(Away) Reporter
	Comment 2 • 10 years ago

> 	netwerk/sctp/datachannel/moz.build
> 39 	NO_PGO = True # Don't PGO
> 	netwerk/sctp/src/moz.build
> 92 	NO_PGO = True # Don't PGO
> Not sure...

Dug a few levels into history, this stems from bug 837035 comment 18. Looks like it was intermittent so I'd want to do a number of mochitest runs to verify.

	Gregory Szorc [:gps]
	Comment 3 • 10 years ago

I think it would be worth your time to add inline comments documenting exactly why we don't PGO. There's enough cargo-culting in the build system already: I don't want deviations from standard behavior going undocumented.

	Makoto Kato [:m_kato]
	Comment 4 • 10 years ago

Also, we should check '#pragma optimize( "", off )' (see bug 723197).

Should I file a new bug for #pragma issue to avoid PGO bugs?

	(Away) Reporter
	Comment 5 • 10 years ago

Good catch! These appear to be almost all PGO-related so I think we can address them in this bug.

http://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=%22pragma%20optimize%22

	(Away) Reporter
	Comment 6 • 10 years ago

Although, for judicious use of '#pragma optimize' around single functions, it's unclear that restoring them will be worth the testing cost. I'll look at them on a case-by-case basis. Some appear to be more easily testable than others.

	Randell Jesup [:jesup] (needinfo me)
	Comment 7 • 10 years ago

(In reply to David Major [:dmajor] (UTC+13) from comment #2)
sctp/datachannels & sctp/src:

> Dug a few levels into history, this stems from bug 837035 comment 18. Looks
> like it was intermittent so I'd want to do a number of mochitest runs to
> verify.

Correct (I just dug it out myself).  The SCTP code in particular has some very large code blocks in c and large switch statements, which can be tricky for PGO...  Of course, they may have fixed it, or we may have fixed whatever timing hole PGO helped trip over.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 8 • 6 years ago

I told dmajor I'd take a look at this once the VS 15.6 update has stuck.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 9 • 6 years ago

Try looks pretty happy with this. I ran 25+ PGO builds to see if any random internal compiler errors showed up during the build. I also ran each test suite 10 times (25 for Jit & mda) without any obvious signs of new issues, only known oranges.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace21ec7830de016d825491608ff2e7ebb9cc9e7

We're early enough in the cycle that I think we can just roll with this and see what shakes out in real builds if there's any lingering concerns about less-than-obvious issues turning up.

	(Away) Reporter
	Comment 11 • 6 years ago

For posterity since some of these didn't have bugs commented:

The sctp ones come from https://bugzilla.mozilla.org/show_bug.cgi?id=837035#c18

xpcom/reflect/xptcall/src/md/unix dates back to bug 569137.

xpcom/reflect/xptcall/src/md/win32 comes from bug 950323.

	(Away) Reporter
	Comment 12 • 6 years ago

Comment on attachment 8958595 [details] [diff] [review]
remove uses of no_pgo that appear to no longer be necessary

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

::: gfx/layers/moz.build
@@ -493,5 @@
> -# Workaround compiler bug (Bug 795594)
> -if CONFIG['CC_TYPE'] in ('msvc', 'clang-cl') and CONFIG['CPU_ARCH'] == 'x86_64':
> -    for src in [
> -        'Layers.cpp',
> -        'LayerTreeInvalidation.cpp',

Now that these files no longer have special flags, they should be moved from SOURCES to UNIFIED_SOURCES.

::: js/src/moz.build
@@ -705,5 @@
>  if CONFIG['CC_TYPE'] in ('msvc', 'clang-cl'):
> -    if CONFIG['CPU_ARCH'] == 'x86':
> -        SOURCES['builtin/RegExp.cpp'].no_pgo = True # Bug 772303
> -    elif CONFIG['CPU_ARCH'] == 'x86_64' and CONFIG['JS_HAS_CTYPES']:
> -        SOURCES['ctypes/CTypes.cpp'].no_pgo = True # Bug 810661

Ditto for RegExp.cpp (and then remove the comment at L404).

Probably the same for CTypes.cpp ...and actually it looks like there's a ton of stuff in this moz.build with no clear reason for using SOURCES. Do you feel like cleaning these up in a followup?

::: xpcom/reflect/xptcall/md/win32/moz.build
@@ -33,5 @@
>              'xptcinvoke.cpp',
>              'xptcinvoke_asm_x86_msvc.asm',
>              'xptcstubs.cpp',
>          ]
> -        SOURCES['xptcinvoke.cpp'].no_pgo = True

This file doesn't use UNIFIED_SOURCES at all... meh, there are so few that I wouldn't bother.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 13 • 6 years ago

(In reply to David Major [:dmajor] from comment #12)
> Now that these files no longer have special flags, they should be moved from
> SOURCES to UNIFIED_SOURCES.
> 
> Ditto for RegExp.cpp (and then remove the comment at L404).

Done, though not without a little excitement along the way. Both of these changes hit bustage requiring additional patches as noted by the deps of this bug.

> Probably the same for CTypes.cpp ...and actually it looks like there's a ton
> of stuff in this moz.build with no clear reason for using SOURCES. Do you
> feel like cleaning these up in a followup?

Filed bug 1445773. I may take a poke at it if nobody else has, but no sweat off my back if someone else wants to work on it.

	Pulsebot
	Comment 14 • 6 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec0f839a905
Remove uses of no_pgo that are no longer needed. r=dmajor

	Pulsebot
	Comment 15 • 6 years ago

Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7762a2ff28
Backed out changeset 1ec0f839a905 for mass Win7 debug test failures.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 16 • 6 years ago

Try is showing quite conclusively that it was the RegExp.cpp move to UNIFIED_SOURCES which caused the mass Win32 slowdown issues that led to yesterday's backout. I'll punt on that change for now and re-land. Bug 1445773 can try to figure out what's going on there.

	Pulsebot
	Comment 17 • 6 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29365e0d62fb
Remove uses of no_pgo that are no longer needed. r=dmajor

	Cristian Brindusan [:cbrindusan]
	Comment 18 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/29365e0d62fb