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

RESOLVED FIXED in Thunderbird 9.0

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: therubex, Assigned: Callek)

Tracking

Trunk
Thunderbird 9.0
x86
Windows 7

Thunderbird Tracking Flags

(thunderbird8 fixed, seamonkey2.5 fixed, seamonkey2.6 fixed)

Details

Attachments

(1 attachment)

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)
This affects Thunderbird as well. And is a simple port, therefore has rs=KaiRo from a past discussion.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Product: SeaMonkey → MailNews Core
QA Contact: build-config → build-config
Summary: Port |Bug 556382 - Link 32-bit Windows builds with LARGEADDRESSAWARE| to SeaMonkey → Port |Bug 556382 - Link 32-bit Windows builds with LARGEADDRESSAWARE| to comm-central
Version: Seamonkey 2.4 Branch → Trunk
Posted patch Patch.Splinter Review
...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.
Attachment #559721 - Flags: approval-comm-beta?
Attachment #559721 - Flags: approval-comm-aurora?
Assignee: nobody → bugspam.Callek
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
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.
Attachment #559721 - Flags: approval-comm-aurora? → approval-comm-aurora+
(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 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.
Attachment #559721 - Flags: approval-comm-beta? → approval-comm-beta-
[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
(In reply to neil@parkwaycc.co.uk 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 on attachment 559721 [details] [diff] [review]
Patch.

Adding back approval flag that was mistakenly removed. dkl
Attachment #559721 - Flags: approval-comm-beta-
You need to log in before you can comment on or make changes to this bug.