Followup: comment 6 in bug 592007. From Igor: In the browser JSGC_MAX_BYTES is 2^32. We should try to reduce this to some sane value as now, without malloced scopes, we have much less malloc pressure. So what is a sane value?

(In reply to comment #0) > Followup: comment 6 in bug 592007. > > From Igor: > In the browser JSGC_MAX_BYTES is 2^32. We should try to reduce this to some > sane value as now, without malloced scopes, we have much less malloc pressure. > > > So what is a sane value? Something that would trigger OOM before exhausting all physical memory, like, for example, 512 MB.

This patch sets JSGC_MAX_BYTES to 512MB for both the browser and the shell. MAX_MALLOC_BYTES is reduced from 128MB to 64MB again.

Comment on attachment 472305 [details] [diff] [review] patch >diff -r 7176e88f36eb dom/base/nsJSEnvironment.cpp >--- a/dom/base/nsJSEnvironment.cpp Sat Sep 04 18:45:26 2010 -0400 >+++ b/dom/base/nsJSEnvironment.cpp Sun Sep 05 15:29:29 2010 -0700 >@@ -4003,17 +4003,17 @@ SetMemoryHighWaterMarkPrefChangedCallbac > * to use jsmalloc() and the other is to use GC-owned memory > * (e.g. js_NewGCThing()). > * > * In the browser, we don't cap the amount of GC-owned memory. > */ Nit: the comment need an update.

updated version

Rob did you land this patch?

Andreas can you land this patch? My connection is Europe is too slow.

Backed out pending measurements and some reasoning why that number is good http://hg.mozilla.org/tracemonkey/rev/f1b428337950

Brian do you have any working set size measurements with your new patch? Or any comments on how the ratio between heap size and mallocs changed after your patch? Otherwise I will start looking at JS heavy sites.

I measured this for V8 in the shell but don't have those numbers anymore. IIRC 20-30MB of memory moved from the malloc heap into the GC heap. For object slots, the main things we still malloc for are large arrays and scripted 'new' objects with more than four properties.

http://pastebin.mozilla.org/817219 An IRC conversation about this bug.

shaver told me no pastebin for IRC... gwagner: sayrer: for bug 592907. what measurements do you wanna see? [2:35pm] sayrer: gwagner: need a rationale for the number [2:35pm] sayrer: gwagner: where did you get 512MB? [2:37pm] gwagner: sayrer: igor suggested it and out of observation that with a bigger heap the GC pause time could become very long [2:38pm] sayrer: is that any different than what we have now? [2:40pm] Wes--: Does Moz have a benchmark for benchmarking minified code, stuff gen'd with Google Gears, etc? [2:40pm] gwagner: sayrer: before the shape patch all the mallocs were the main GC points. this is gone and now we have sites where the gc heap grows up to 1.5GB [2:41pm] gwagner: sayrer: sorry the memory that the browser uses. not GC heap [2:41pm] Wes--: Doesn't GC trigger everytime the heap increases by 1600% since last GC anyhow? [2:41pm] sayrer: gwagner: don't we GC everytime the GC heap grows by 1.5x? [2:42pm] sayrer: I thought that was what the JSGCArena check did [2:42pm] Wes--: sayrer: not 1.5, you can't set that tunable any lower than 200, it's 300 in the mobile browser and 1600 in firefox if I followed previous discussions correctly [2:43pm] sayrer: oh right, 1.5 x N [2:43pm] gwagner: sayrer: this was too small for benchmarks. this is 3x now [2:44pm] dherman: cdleary: have you had a chance to look over bug 594060? are you waiting on other changes from me? [2:44pm] sayrer: gwagner: when did that change? [2:45pm] gwagner: sayrer: soon after my initial fix because we called the GC too often for the benchmarks [2:45pm] dherman: thx, no worries [2:45pm] gwagner: sayrer: this was too restricted for splay [2:45pm] sayrer: gwagner: so how is it we can call the GC too much for these benchmarks, but still need this limit? [2:46pm] gwagner: sayrer: this limit is not for the benchmarks. it's for allocation heavy sites [2:46pm] sayrer: I think my question stands [2:46pm] sayrer: we GC during SunSpider, for instance [2:47pm] sayrer: so, what' s the problem this limit will solve? [2:47pm] gwagner: sayrer: do you mean the upper heap limit or the growth limit? [2:47pm] sayrer: the upper heap limit [2:48pm] gwagner: so the limit solves the problem that we can be more aggressive with the growth limit [2:48pm] sayrer: explain more [2:49pm] gwagner: otherwise we end up with a huge heap size for allocation heavy benchmarks like the realtime raytracer [2:49pm] sayrer: ok, let's use raytracer [2:49pm] sayrer: what happens now, and what is the new proposal? [2:49pm] gwagner: lets say the raytracer has a working set of 200MB. we end up with 600MB JS heap [2:50pm] gwagner: the overall browser memory usage is up to 1GB or more [2:50pm] gwagner: so I propose a more relaxed growth limit but a strict upper bound that solves the problem if the working set is already high [2:51pm] sayrer: why wouldn't we change the curve of the growth limit as it gets bigger [2:52pm] shaver: hmm [2:52pm] gwagner: thats also possible [2:52pm] shaver: naively, I would think that as our heap gets larger, it's more important for us to collect frequently [2:52pm] shaver: because excess garbage is more likely to push us OOM (or out of address space) [2:52pm] sayrer: I gotta go, but let's get this in the bug (none of it is

Lets split the problem up in smaller parts: I filed bug 604747 to increase the GC heap size for the shell. In addition I filed bug 604749 to get rid of extra GC runs in the shell. This bug should only focus on the upper limit for the browser GC heap.

I've attached a test page, which just allocates a large array. In firefox4b7, I can allocate a 4GB array, which ruins browsing, and using my 4GB machine, for the next five minutes (assuming I've clicked the 'this script is slow -> OK' button). This might be considered a security issue: a random site can DOS my machine. By contrast, Chrome crashes the tab after trying to allocate a array larger than 32MB (minus some change). I'm not sure what figures are required here, perhaps "how much do large sites currently use?". But in the absence of such figures, we still need a reasonable limit here. 300MB should be plenty, certainly not more than 512MB though (and we should probably revisit this limit every few months). It might break on a really heavy site like graphs.mozilla.org, but that would be a feature, not a bug.

(In reply to comment #14) > Created attachment 490877 [details] > HTML page allocating large array > > I've attached a test page, which just allocates a large array. In firefox4b7, I > can allocate a 4GB array, which ruins browsing, and using my 4GB machine, for > the next five minutes (assuming I've clicked the 'this script is slow -> OK' > button). This might be considered a security issue: a random site can DOS my > machine. This is definitely an important point! Hopefully we can catch this within a single compartment pretty soon.

(In reply to comment #14) > Created attachment 490877 [details] > HTML page allocating large array > > I've attached a test page, which just allocates a large array. In firefox4b7, I > can allocate a 4GB array, which ruins browsing, and using my 4GB machine, for > the next five minutes (assuming I've clicked the 'this script is slow -> OK' > button). This might be considered a security issue: a random site can DOS my > machine. I looked at this in more detail and I don't think we can solve this problem with our current limits. Maybe I am missing something? This array creates a single object with 0 fslots on the heap. So all the heap-size limits are not involved at all. The size of the array will trigger a GC because we overflow the MAX_MALLOC_BYTES limit. After the GC, we reset the gcMallocBytes counter and we completely forget that we just allocated a huge array.

(In reply to comment #14) > This might be considered a security issue: a random site can DOS my machine. Note that denial of service on its own is not considered a security issue. Quoting from bug 538085 comment 15 (now hidden, perhaps over-cautiously but I'm not going to push it): > But, denial of service in the browser, if that's all that's present, is not > considered a security issue in and of itself. There are a million different > ways to crash the browser, and choosing to escalate the priority of a game > of whack-a-mole against deliberate attempts to do so doesn't make much sense. > Users will stop visiting sites that make such deliberate attempts; it's a > self-limiting problem. Better to spend time on the crashes encountered by > well-behaving sites. DoS bugs can be frustrating, to be sure, but it's not > productive to treat them as security issues. One might perhaps distinguish full-system DoS from browser-only DoS, but I can't remember our having made this distinction before. I don't doubt there are many other ways to do this, however, so I suspect the distinction fails as a practical matter for much the same reason DoS-as-security-issue fails.

Looks like the code in question has seen some changes in the last year. Still a valid non-TM bug?

This has been addressed in other bugs.