686139 - Port |Bug 556382 - Link 32-bit Windows builds with LARGEADDRESSAWARE| to comm-central

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[therube]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20110905 Firefox/7.0 SeaMonkey/2.4
Build ID: 20110905000422

Steps to reproduce:

 
Appears that SeaMonkey 32-bit Windows builds are not being built with linker flag /LARGEADDRESSAWARE ?

(IMAGE_FILE_LARGE_ADDRESS_AWARE /LARGEADDRESSAWARE)

Comment 1

[Justin Wood (:Callek)]

This affects Thunderbird as well. And is a simple port, therefore has rs=KaiRo from a past discussion.

Comment 2

[Justin Wood (:Callek)]

Attachment: Patch.

...Landed as: http://hg.mozilla.org/comm-central/rev/7a8c6be2d003

Requesting a-beta/aurora (will really help SeaMonkey on those branches at least). But actual approval falls into Thunderbird's court.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 559721 [details] [diff] [review]
Patch.

So I think this is fine for aurora as we've got plenty of time for testing on there.

I think we're rather late in the beta cycle for this, and given the implied memory performance improvements in gecko for 7, I'm thinking about saying no - although it seems like a simple change, it is still a build config change that won't have had much testing.

Comment 4

[Justin Wood (:Callek)]

(In reply to Mark Banner (:standard8) from comment #3)
> Comment on attachment 559721 [details] [diff] [review]
> Patch.
> 
> So I think this is fine for aurora as we've got plenty of time for testing
> on there.
> 
> I think we're rather late in the beta cycle for this, and given the implied
> memory performance improvements in gecko for 7, I'm thinking about saying no
> - although it seems like a simple change, it is still a build config change
> that won't have had much testing.

I think for this, we should still accept on comm-beta, its been in m-beta for a while, also is an extremely safe feature we can bind an application with. (the only potential downside relates to perpetually bad use of pointer values, and is why its not default-on)

From MSDN (http://msdn.microsoft.com/en-us/library/wz223b1z%28v=vs.80%29.aspx):
The /LARGEADDRESSAWARE option tells the linker that the application can handle addresses larger than 2 gigabytes. By default, /LARGEADDRESSAWARE:NO is enabled if /LARGEADDRESSAWARE is not otherwise specified on the linker line.

So we already expose it in anything linked in mozilla proper (such as xul.dll which already has the mailnews code I could be most persuaded to be scared of hurting)

Comment 5

[Justin Wood (:Callek)]

http://hg.mozilla.org/releases/comm-aurora/rev/96acac491c88

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 559721 [details] [diff] [review]
Patch.

Whilst this does seem low risk, there also seems to be very likely little benefit as libxul is already capable, so in theory this would just be memory allocated in the exe which isn't likely to hit the limit.

Therefore I think we might as well push this out to the next release.

Comment 7

[[e-mail]]

[FYI only. This is not a request for a review of the analysis.]

(In reply to Mark Banner from comment #6)
> Whilst this does seem low risk, there also seems to be very likely little
> benefit as libxul is already capable, so in theory this would just be memory
> allocated in the exe which isn't likely to hit the limit.
That makes no sense. See http://blogs.msdn.com/b/oldnewthing/archive/2010/09/22/10065933.aspx

Comment 8

[Justin Wood (:Callek)]

(In reply to [e-mail] from comment #7)
> [FYI only. This is not a request for a review of the analysis.]
> 
> (In reply to Mark Banner from comment #6)
> > Whilst this does seem low risk, there also seems to be very likely little
> > benefit as libxul is already capable, so in theory this would just be memory
> > allocated in the exe which isn't likely to hit the limit.
> That makes no sense. See
> http://blogs.msdn.com/b/oldnewthing/archive/2010/09/22/10065933.aspx

Given that, it looks like this actually would have been beneficial on beta, but too late now.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 559721 [details] [diff] [review]
Patch.

Adding back approval flag that was mistakenly removed. dkl