Closed Bug 941804 Opened 6 years ago Closed 6 years ago

Allow some compile-time configurability of chunk size

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mccr8, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 2 obsolete files)

1mb is a lot for B2G, so it would be nice if this could be changed at compile time to, say, 250kb.
Blocks: 941783
In bug 941837, billm points out a previous bug that attempted to reduce chunk size to 64k, bug 671702.
> In bug 941837, billm points out a previous bug that attempted to reduce
> chunk size to 64k, bug 671702.

The results there were very mixed, but it might be worth trying again with B2G-specific workloads.
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch try-changing-chunk-size (obsolete) — Splinter Review
Here's a patch to test changing the chunk size to 256K.

Do we want this to be a configure option or will it be sufficient to have ifdefs for B2G do you think?

Also I'm unsure as to how to measure the impact of this for B2G.
Attachment #8453766 - Flags: feedback?(terrence)
I think as a first pass it would be fine to just do #ifdef B2G. Why do we need to move all of our declarations into the API? I think we should be able to make the appropriate changes internally in Heap.h and just have an #ifdef B2G block in HeapAPI.h with the different numbers.
Attached patch reduce-chunk-size-for-b2g (obsolete) — Splinter Review
For some reason I couldn't get this to work with #ifdef MOZ_B2G.  I don't understand how all the configure stuff works, but it seems this isn't propagated when building the engine, at least not for desktop B2G builds.
Assignee: nobody → jcoppeard
Attachment #8453766 - Attachment is obsolete: true
Attachment #8453766 - Flags: feedback?(terrence)
Attachment #8454547 - Flags: review?(terrence)
Comment on attachment 8454547 [details] [diff] [review]
reduce-chunk-size-for-b2g

Review of attachment 8454547 [details] [diff] [review]:
-----------------------------------------------------------------

Great! That's more like what I was thinking.
Attachment #8454547 - Flags: review?(terrence) → review+
Nicolas, what would be the best way to figure out the impact of this change on B2G?
Flags: needinfo?(nicolas.b.pierron)
sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43733909&tree=Mozilla-Inbound
Sorry, it seems I uploaded the wrong version of the patch for review.  Comment 5 should make more sense now.
Attachment #8454547 - Attachment is obsolete: true
Attachment #8455235 - Flags: review?(terrence)
Comment on attachment 8455235 [details] [diff] [review]
reduce-chunk-size-for-b2g v2

Review of attachment 8455235 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, makes sense.
Attachment #8455235 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #7)
> Nicolas, what would be the best way to figure out the impact of this change
> on B2G?

Octane does not run anymore on Unagis, but I have 2 flame which are constantly running.  One has 1 GB and the other runs with 512 mb of RAM.  AreWeFastYet [1] should report how these patches change the behaviour on benchmarks.

For the memory aspect of the question, at the moment you can have a look at datazilla (select one of the *_memory tests on the bottom left box) to see what is the start-up memory of one application [2].  Note that the concern is on Gecko master, and not inbound, so only expect to see the variations after the merge.

[1] http://arewefastyet.com/?a=b&view=regress#machine=26 (due to an adb issue, I was unable to report anything before this morning)
[2] https://datazilla.mozilla.org/b2g/?branch=master&device=flame&range=7&test=settings_memory&app_list=browser,calendar,camera,clock,contacts,email%20FTU,fm_radio,gallery,marketplace,messages,music,phone,rss,settings,system_rss,system_uss,system_vsize,template,usage,uss,video,vsize&app=clock&gaia_rev=40cac290f0a3253d&gecko_rev=c11ea2f54a6a&plot=avg
Flags: needinfo?(nicolas.b.pierron)
Great! That's extremely helpful!
https://hg.mozilla.org/mozilla-central/rev/21f83c1eba92
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
This appears to have resulted in a sharp drop to system rss, uss, and pss of about 50MiB. Full of win!
Note: I'm assuming for now that the scale bars are actually in MiB, not KiB, because a VmSize of 400KiB is flat-out impossible.
You need to log in before you can comment on or make changes to this bug.