Closed Bug 556382 Opened 14 years ago Closed 14 years ago

Link 32-bit Windows builds with LARGEADDRESSAWARE

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(blocking2.0 betaN+)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mozilla, Assigned: Mitch)

References

Details

Attachments

(1 file)

Patch v1
2.59 KB, patch
khuey
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

Microsoft continues to work to limit applications usage of more memory and now require applications to be compiled with a switch "IMAGE_LARGE_ADDRESS_AWARE", in order for a 32-bit application to be *able* to use more than 2GB.

This issue is only important as long as FF remains 32-bit  on 64-bit machines,
as when FF goes 64-bit, the problem will go away naturally.

However, the change NOW, is trivial: just adding a flag to the linker
"/LARGEADDRESSAWARE" -- and FF will be "enabled".  Users can make use of this
via their OS-boot time options.  
----
See option "User-mode virtual address space for each 32-bit process" under
column "Limit in 64-bit Windows".

http://msdn.microsoft.com/en-us/library/aa366778%28VS.85%29.aspx




Reproducible: Always

Steps to Reproduce:
1.  FF uses only 2GB, if switch isn't set.
2.  if linked with the switch, FF is *allowed* to use up to 4GB (in 64-bit windows).
3.
Is this something worth doing?
Status: UNCONFIRMED → NEW
Component: Preferences → Build Config
Ever confirmed: true
Product: Firefox → Core
QA Contact: preferences → build-config
Summary: link-switch@build time to allow 4GB addr usage: please add LARGEADDRESSAWARE to link in 32bit Win builds → Link 32 bit Windows builds with LARGEADDRESSAWARE
I'm not sure what the actual consequences are. There might need to be code auditing before we could flip it. (I'm also not clear that there's a huge upside here. Does anyone want their browser using >2GB of memory?)
Blocks: 590674
As explained in bug 590674 this is an actual issue as FF4 OOM-crashes around 1.5GB under heavy load on a 64bit windows system instead of using the theoretically available 4GB.
Since no 64bit release of FF4 planned yet for windows this might be a viable workaround.
Note that this isn't 2GB allocated but 2GB of _address_space_ in use.  As in, we can get there due to fragmentation, random large memory mappings from someone, etc...

We've had a number of reports of "Firefox crashes with OOM without actually using as much RAM as I have", and I think we do want to fix this for Gecko 2.0.
blocking2.0: --- → ?
We had a known leak of VM earlier in the cycle, but that's fixed. I really don't think this is serious enough to block a release.
blocking2.0: ? → -
> We had a known leak of VM earlier in the cycle, but that's fixed

Yes, but people are still commonly hitting crashes at 1.5GB of memory usage....
a) this still occurs with FF4b6, actually it happens more often due to increased memory usage. So it's probably unrelated to the memory leak
b) it happens right after startup when restoring a large session (200+ tabs), so it's not something that slowly leaks over time.

Right now i'm mostly sticking to FF3 since it uses less memory with the same amount of tabs and therefore does not run into this memory barrier.

Just to emphasize: If i take my FF3 sessionrestore as-is (it uses around 1GB ram) and move it to a clean FF4b6 profile it shoots up to about 1.5GB of memory on startup and OOM-crashes.
Ah, and some other aspects that should be considered

a) I'm aware that heavy tab usage is not the norm at the moment, but this may change since tabcandy is designed to make a large number of tabs more manageable
b) Whenever i get an OOM crash the crash reporter says "there was an error submitting your crash report", which means that the frequency of these crashes and thus the severity of the issue may be underreported.
Firefox 4 using more memory should be a separate bug. I might block on that. But I'm not going to block on just adding an extra 1G of address space to the Firefox process.
We use more address space, obviously.  For one thing, we're mapping graphics card memory via some of our hardware acceleration stuff and so forth....  That doesn't seem like something we should block on, to me, whereas increasing our address space to handle these new mappings does.
Are we doing that per-tab or per-window? It seems to me that keeping graphics card memory alive for hidden tabs is a bad tradeoff, if that's what we're doing.
I have no idea, nor whether we can even control it very well; ccing some graphics folks.
(In reply to comment #11)
> We use more address space, obviously.  For one thing, we're mapping graphics
> card memory via some of our hardware acceleration stuff and so forth....

It's not just gfx i think. After some suggestions i changed to the following settings:

image.mem.decodeondraw = true
image.mem.discardable = true
gfx.direct2d.disabled = true
gfx.font_rendering.directwrite.enabled = false
mozilla.widget.render-mode = 0

with these settings i still get the behavior described in comment #8
> Just to emphasize: If i take my FF3 sessionrestore as-is (it uses around 1GB
> ram) and move it to a clean FF4b6 profile it shoots up to about 1.5GB of memory
> on startup and OOM-crashes.

Just as a comparison:

FF3.6 + 270 tabs = 1 to 1.2GB
FF4b4 + 170 tabs = 1.4GB+ => crash
FF4b4 + 170 tabs = 1.2GB (with image.mem settings)
FF4b6 + 170 tabs = 1.6GB (with image.mem settings + HW acceleration off) => crash
(In reply to comment #10)
> Firefox 4 using more memory should be a separate bug. I might block on that.
> But I'm not going to block on just adding an extra 1G of address space to the
> Firefox process.
on a 64bit system it would actually add 2GB of address space.
(In reply to comment #12)
> Are we doing that per-tab or per-window? It seems to me that keeping graphics
> card memory alive for hidden tabs is a bad tradeoff, if that's what we're
> doing.

We aren't. We are using more memory per window, though.
(In reply to comment #10)
> But I'm not going to block on just adding an extra 1G of address space to the
> Firefox process.

(In reply to comment #11)
> That doesn't seem like something we should block on, to me,
> whereas increasing our address space to handle these new mappings does.

Those are basically 2 different solutions to the same problem: OOM crash due to increased virtual address space use.

So I'll file an additional bug about increased memory usage, make bug 590674 depend on it too and request blocking2.0 on it instead, which should cover both points of view. Which way it'll be resolved ultimately shouldn't matter as long as it gets done.
(In reply to comment #10)
> Firefox 4 using more memory should be a separate bug.

done: bug 598466
so... since nobody seems to be working on this i tried it myself. downloaded visual studio, applied editbin.exe /largeaddressaware <path to firefox>\firefox.exe

And voila, firefox is using 2GB private or working set and 2.6GB virtual address space and no more OOM-crashes.
we should do this. i don't believe we have any code which does evil things to pointers and expects to only be in the lower 2gb of space.

because of the additional stuff we map in, our usable space for normal operations has indeed shrunken relative to previous versions.

there is no affect on systems which aren't 64bit or don't boot with the /3gb switch. so it's safe for "average cases". and it's the right path for bigger/newer systems.

it's also cheap to enable (as comment 19 notes, although one can do it as part of the build process instead of later).
I've been using the nightlies for a month now and applied the /LARGEADDRESSAWARE switch every time it updates and it has been using > 3GB virtual address space without issues.

So from my point of view it would seem reasonable to include this in the build process.
Everyone agrees we want this. It's only waiting for somebody to write the patch.
Attached patch Patch v1 — — Splinter Review 
I can regularly reproduce crashes without this fix at ~1.2GB memory used by the process. With the fix, I stress tested it with hundreds of sites/pages open, and process memory usage went over 2.1GB. Firefox paused a lot and was generally sluggish but did not crash.
Assignee: nobody → mitchell.field
Status: NEW → ASSIGNED
Attachment #488740 - Flags: review?(benjamin)
Severity: normal → major
Summary: Link 32 bit Windows builds with LARGEADDRESSAWARE → Link 32-bit Windows builds with LARGEADDRESSAWARE
Version: unspecified → Trunk
Comment on attachment 488740 [details] [diff] [review]
Patch v1

Why are we testing for MSC_VER >= 1400?  Isn't Visual Studio 2005 the minimum required version on trunk these days?

r=me regardless.
Attachment #488740 - Flags: review?(benjamin) → review+
(In reply to comment #20)
> we should do this. i don't believe we have any code which does evil things to
> pointers and expects to only be in the lower 2gb of space.
> 
> because of the additional stuff we map in, our usable space for normal
> operations has indeed shrunken relative to previous versions.
> 
> there is no affect on systems which aren't 64bit or don't boot with the /3gb
> switch. so it's safe for "average cases". and it's the right path for
> bigger/newer systems.
> 
> it's also cheap to enable (as comment 19 notes, although one can do it as part
> of the build process instead of later).

This might affect crappy binary extensions.  That's not a reason not to do it IMO, but there will be >0 fallout.
blocking2.0: - → ?
(In reply to comment #25)
> This might affect crappy binary extensions.  That's not a reason not to do it
> IMO, but there will be >0 fallout.
problems would only occur when the virtual address space exceeds the 2GB limit, in which case it would crash anyway.
technically such extensions could crash in "interesting" ways instead of harmless ways...

but yeah, i'd like to see this in.
Attachment #488740 - Flags: approval2.0?
Attachment #488740 - Flags: approval2.0?
(In reply to comment #24)
> Why are we testing for MSC_VER >= 1400?  Isn't Visual Studio 2005 the minimum
> required version on trunk these days?
Actually even VC6 supports -LARGEADDRESSAWARE anyway.
This patch breaks the compilation of libffi on our (Instantbird) Windows builder.
(See http://buildbot.instantbird.org/builders/win32-nightly-default/builds/301/steps/compile/logs/stdio for the build log)

This seems somewhat similar to bug 590996.
I wonder why we don't see that on m-c.
(In reply to comment #31)
> I wonder why we don't see that on m-c.

I don't know. Bug 590996 is not visible on m-c either (on the tinderbox I mean). I initially thought it was because we still build with VS2005 and m-c was built with a newer version, but Shaver said some tinderboxes run VS2005.
All of our 32 bit Windows builds are done with VS2005.

Mitch, can we resolve this bug?
I was only waiting for the builds. Resolving as fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: