Last Comment Bug 751695 - Make our builders pick the correct mozconfig on Windows based on whether they're on a 32-bit or a 64-bit platform
: Make our builders pick the correct mozconfig on Windows based on whether they...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: PGOSilverBullet 760117
  Show dependency treegraph
 
Reported: 2012-05-03 13:36 PDT by :Ehsan Akhgari
Modified: 2012-05-31 07:51 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.34 KB, patch)
2012-05-03 14:26 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (3.05 KB, patch)
2012-05-04 12:04 PDT, :Ehsan Akhgari
ted: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-05-03 13:36:07 PDT
I'm looking for a way to make our default Win32 mozconfigs do the right thing based on whether the builder is a Win32 or a Win64 builder.  This should make it possible for RelEng to gradually move our Windows builds to the Win64 builders, so that we can take advantage of the additional 1G of address space.
Comment 1 :Ehsan Akhgari 2012-05-03 14:26:02 PDT
Created attachment 620854 [details] [diff] [review]
Patch (v1)
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-05-04 04:48:27 PDT
Comment on attachment 620854 [details] [diff] [review]
Patch (v1)

This seems like it'd be more simply implemented as:

if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" = "AMD64"; then
Comment 3 JP Rosevear [:jpr] 2012-05-04 07:16:11 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #0)
> I'm looking for a way to make our default Win32 mozconfigs do the right
> thing based on whether the builder is a Win32 or a Win64 builder.  This
> should make it possible for RelEng to gradually move our Windows builds to
> the Win64 builders, so that we can take advantage of the additional 1G of
> address space.

Are we sure we want to do this incrementally?  Generating builds on two different OSes seems a little scary.
Comment 4 :Ehsan Akhgari 2012-05-04 07:51:29 PDT
(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 620854 [details] [diff] [review]
> Patch (v1)
> 
> This seems like it'd be more simply implemented as:
> 
> if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" =
> "AMD64"; then

I tested this.  Those variables don't seem to be reliably set for some reason.  Using IsWow64Process is a much more reliable way to gather this information.
Comment 5 :Ehsan Akhgari 2012-05-04 07:53:07 PDT
(In reply to JP Rosevear [:jpr] from comment #3)
> (In reply to Ehsan Akhgari [:ehsan] from comment #0)
> > I'm looking for a way to make our default Win32 mozconfigs do the right
> > thing based on whether the builder is a Win32 or a Win64 builder.  This
> > should make it possible for RelEng to gradually move our Windows builds to
> > the Win64 builders, so that we can take advantage of the additional 1G of
> > address space.
> 
> Are we sure we want to do this incrementally?  Generating builds on two
> different OSes seems a little scary.

I'm not convinced that it is, really.  We're using the same compiler, the only thing which is different is the OS CPU architecture, which should not make any difference in the code that the compiler generates.  Also, note that we'll at least switch all PGO builds to Win64 builders, which means that all of our release builds are always generated on Win64 machines, so technically we'll be using the same OS to build all release binaries.
Comment 6 :Ehsan Akhgari 2012-05-04 07:53:29 PDT
(By "release" I mean Nightly, Aurora, Beta and Firefox.)
Comment 7 Nathan Froyd [:froydnj] 2012-05-04 08:01:07 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> (In reply to JP Rosevear [:jpr] from comment #3)
> > Are we sure we want to do this incrementally?  Generating builds on two
> > different OSes seems a little scary.
> 
> I'm not convinced that it is, really.  We're using the same compiler, the
> only thing which is different is the OS CPU architecture, which should not
> make any difference in the code that the compiler generates.

Famous last words. ;)  A number of things can go wrong with compiler reproducibility; I do think it'd be prudent to compare Win32-built binaries with Win64-built binaries before checking this in.

> Also, note
> that we'll at least switch all PGO builds to Win64 builders, which means
> that all of our release builds are always generated on Win64 machines, so
> technically we'll be using the same OS to build all release binaries.

This does lower the risk factor somewhat.  But it still leaves open weird build machine-dependent bugs (on try, for instance) during the switchover.
Comment 8 :Ehsan Akhgari 2012-05-04 08:16:19 PDT
(In reply to Nathan Froyd (:froydnj) from comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > (In reply to JP Rosevear [:jpr] from comment #3)
> > > Are we sure we want to do this incrementally?  Generating builds on two
> > > different OSes seems a little scary.
> > 
> > I'm not convinced that it is, really.  We're using the same compiler, the
> > only thing which is different is the OS CPU architecture, which should not
> > make any difference in the code that the compiler generates.
> 
> Famous last words. ;)  A number of things can go wrong with compiler
> reproducibility; I do think it'd be prudent to compare Win32-built binaries
> with Win64-built binaries before checking this in.

For sure.  That's what bug 751694 is about.  We can also test this on the try server once this patch is in.

But do you have any specific concerns?

> > Also, note
> > that we'll at least switch all PGO builds to Win64 builders, which means
> > that all of our release builds are always generated on Win64 machines, so
> > technically we'll be using the same OS to build all release binaries.
> 
> This does lower the risk factor somewhat.  But it still leaves open weird
> build machine-dependent bugs (on try, for instance) during the switchover.

I'm not sure what you mean.  We already have the problem of PGO specific bugs which do not show up in our non-PGO testing builds which we do much more regularly on central/inbound.  So in a sense the risk of the difference between PGO builds on Win64 builders and non-PGO builds on Win32 builders is something that we're already dealing with, roughly speaking.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-05-04 08:22:05 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> I tested this.  Those variables don't seem to be reliably set for some
> reason.  Using IsWow64Process is a much more reliable way to gather this
> information.

Really? They work fine from both cmd and msys on my Windows 7 x64. These are actually recommended by MSDN!
Comment 10 :Ehsan Akhgari 2012-05-04 08:25:05 PDT
(In reply to Ted Mielczarek [:ted] from comment #9)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > I tested this.  Those variables don't seem to be reliably set for some
> > reason.  Using IsWow64Process is a much more reliable way to gather this
> > information.
> 
> Really? They work fine from both cmd and msys on my Windows 7 x64. These are
> actually recommended by MSDN!

When I tested this yesterday on the builder, it did not work.  I gave up access to the builder, but I can file a new bug to get access to another builder and retest if you think this is worth it.
Comment 11 Mozilla RelEng Bot 2012-05-04 09:00:20 PDT
Try run for 3b50adb710a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3b50adb710a5
Results (out of 2 total builds):
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-3b50adb710a5
Comment 12 Mozilla RelEng Bot 2012-05-04 12:00:28 PDT
Try run for 3db3cd41f7c8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3db3cd41f7c8
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-3db3cd41f7c8
Comment 13 :Ehsan Akhgari 2012-05-04 12:04:42 PDT
Created attachment 621123 [details] [diff] [review]
Patch (v2)

Using environment variables.
Comment 14 Ted Mielczarek [:ted.mielczarek] 2012-05-04 12:09:22 PDT
Comment on attachment 621123 [details] [diff] [review]
Patch (v2)

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

::: browser/config/mozconfigs/win32/debug
@@ +10,5 @@
> +if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" = "AMD64"; then
> +  . $topsrcdir/build/win32/mozconfig.vs2010-win64
> +else
> +  . $topsrcdir/build/win32/mozconfig.vs2010
> +fi

Kind of wondering if this doesn't deserve to be in its own separate file that then sources the proper file. Am I overcomplicating things?
Comment 15 :Ehsan Akhgari 2012-05-04 12:21:00 PDT
(In reply to Ted Mielczarek [:ted] from comment #14)
> Comment on attachment 621123 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 621123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/config/mozconfigs/win32/debug
> @@ +10,5 @@
> > +if test "$PROCESSOR_ARCHITECTURE" = "AMD64" -o "$PROCESSOR_ARCHITEW6432" = "AMD64"; then
> > +  . $topsrcdir/build/win32/mozconfig.vs2010-win64
> > +else
> > +  . $topsrcdir/build/win32/mozconfig.vs2010
> > +fi
> 
> Kind of wondering if this doesn't deserve to be in its own separate file
> that then sources the proper file. Am I overcomplicating things?

Yes!  :P
Comment 17 Ed Morley [:emorley] 2012-05-05 03:40:40 PDT
https://hg.mozilla.org/mozilla-central/rev/d944ef78da84

Note You need to log in before you can comment on or make changes to this bug.