TM: shell GC heap size should match browser heap size

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

unspecified
mozilla9
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

7 years ago
Assignee: general → anygregor
(Assignee)

Comment 1

7 years ago
Created attachment 483580 [details] [diff] [review]
patch

This patch sets the heap size of the shell to 512MB and gcMaxMallocBytes back to 64MB.
(Assignee)

Comment 2

7 years ago
Created attachment 483581 [details] [diff] [review]
patch
Attachment #483580 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Blocks: 604749

Comment 3

7 years ago
I think this is important to get implemented quickly, for multiple reasons:

1.  Test environment should mimic actual browser as closely as possible
2.  Using the data, the browser can be tuned for better real-world performance
3.  Many people, including (until recently) myself, took AWFY at face value.  If the shell is not truly representative of real-world browser behavior, aren't we doing ourselves an injustice by using it as the "benchmark"?

Are there any other areas and/or settings where the shell is not representative of the browser?  Are there plans (bugs filed) to make it so?

If I am out of line here, I apologize.  It just seems to me that we should be measuring progress based on what users will experience, not just in a test environment.

Paul
(Assignee)

Comment 4

7 years ago
I was absolutely sure that increasing the JS heap size makes sense... Until I did the measurements for bug 606637.
There we see that the benchmarks for the shell and the browser have totally different memory requirements.
So you are absolutely right with your questions!

We can avoid all GC runs during the v8 benchmark in the shell version and make it faster but there is no way to avoid them in the browser version.

Fact is that the browser versions of the benchmarks are more important than the shell versions. 
Removing all GC runs in the shell version also means that we don't see GC regressions right away.

There is a tradeoff between:
1) Yes it makes sense to increase the heap size because we would see how the VM would handle a bigger heap. And yes we are faster on AWFY.

2) No it makes no sense because the working sets for all the shell programs is too small to mimic the browser benchmark environment. Furthermore we don't see the GC performance in the benchmarks that we run every day.

Another major problem is that the shell versions of the benchmarks don't mimic the actual browser versions. And even worse, the browser versions don't even mimic the actual web behavior... So what are we tuning for? :)

I could write a big section about how I think the compartmental GC will help  but lets wait for the real numbers.

Back to this bug: This patch is ready but I am not so sure if this is the way to go.

Comment 5

7 years ago
We have 2 separate harnesses and issues here:

- The sunspider web harness runs short little benchmarks that don't do much. We should make sure we don't hit a last ditch while the benchmarks run. Instead, we should schedule preventive GCs at better times. This will help web content, too.

- The v8 web harness measures rates, and is mostly a GC benchmark. It creates tons of data and uses V8's generational GC to look fast. Yay! We can't win this competition by increasing the heap size. The compartmental GC will make us faster on last ditch GCing, and should help us with the v8 web harness.

Executive summary: lets make sure we don't GC during sunspider harness runs. The v8 harness is secondary and we will deal with it for b8.
Created attachment 560668 [details] [diff] [review]
patch to set maxheap to 4GB

I recently found that the difference in maxheap between the shell and browser was causing massive differences in performance on splay. This patch sets maxheap to 4GB, the same as it is in the browser. With the patch, the shell and browser now give me the same score. (This is using the shell version of V8bench that is packaged with V8 as benchmarks/run.js, not the crappy SunSpider version.)

I think we should take this because it makes benchmarking in the shell more realistic.
Attachment #560668 - Flags: review?(anygregor)
(Assignee)

Comment 7

6 years ago
Comment on attachment 560668 [details] [diff] [review]
patch to set maxheap to 4GB

lets try it!
Attachment #560668 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9208ee94b012

I ended up changing this to be closer to what the browser does. It allocates the runtime with a 32MB max heap size, and then increases it to 4GB. This way max malloc bytes gets set to 32MB rather than 4GB (which would be undesirable).
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/9208ee94b012
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.