Closed Bug 420476 Opened 17 years ago Closed 13 years ago

jsarena code allocates too much due to alignment math

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED DUPLICATE of bug 675545

People

(Reporter: moz, Assigned: moz)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 4 obsolete files)

User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 Build Identifier: The code in jsarena.c adds overhead to allocations to account for alignment. The code currently adds too many bytes in some cases, resulting in unnecessarily large mallocations. See 'gross' in JS_ArenaAllocate, and consider the case of nonzero pool->mask. Reproducible: Always
Assignee: general → moz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Priority: -- → P3
Attached patch Sneak peak patch (obsolete) (deleted) — Splinter Review
This patch provides a sneak peak at what I'm doing in this bug. The patch works. It reduces the 'gross' size requested from malloc()s. It also makes code changes which clarify the alignment math that was previously allocating slightly too much in some circumstances. This patch is relative to changes that I made in my last patch in bug 415967. We expect the memory use impact of this patch to be negligible relative to that of the patch in bug 415967. It's the latter that saves us the memory. This patch just fixes the confusing alignment math. I'll post memstats soon. Note that the changes in this patch should make it very easy to fix bug 408921. That bug is the real motivator for this patch, in fact.
Attached patch Patch (relative to patch from bug 415967) (obsolete) (deleted) — Splinter Review
This patch is a completed version of the "sneak peak" patch. The patch is relative to the patch from bug 415967. I will upload the real patch when bug 415967 is resolved. Stats coming up.
Attachment #307750 - Attachment is obsolete: true
Attachment 307817 [details] [diff] yields measurable results on a 'start and then stop the browser' test: Before the patch: numMallocs: 6355 numReallocs: 5 numFrees: 6355 hwmRequested: 332818 hwmReal: 374560 After the patch: numMallocs: 6354 numReallocs: 5 numFrees: 6354 hwmRequested: 324176 hwmReal: 365840 So, a mild improvement in allocated memory. More interesting are the histograms of memory allocations, which is we this patch is intended to fix. In the histograms, the value ((x,y),z) means that an allocation of size x was made, and it really had size y when you include the malloc overhead; and there were z such allocations. Before patch: (275,288),297 (303,304),1 (671,1024),1 (1040,1536),1499 (1047,1536),4396 (2075,2560),5 (8211,8704),37 (12307,12800),124 After the patch: (272,272),296 (300,304),1 (668,1024),1 (1040,1536),5895 (2072,2560),5 (8208,8704),38 (12304,12800),123 Of course, this is not the same kind of improvement as with the patch from bug 415967.
Blocks: 419399
I have made a further modification to the code, in order to propose a solution to bug 408921. On top of my prior patches, I also make this change: pool->first.base = pool->first.avail = pool->first.limit = JS_ARENA_ALIGN(pool, &pool->first + 1); pool->current = &pool->first; - pool->arenasize = JS_ARENA_ALIGN(pool, size); + JS_ASSERT(META_SIZE < size); + pool->arenasize = JS_ARENA_ALIGN(pool, size) - META_SIZE; pool->quotap = quotap; #ifdef JS_ARENAMETER memset(&pool->stats, 0, sizeof pool->stats); then bug 408921 goes away, and memory use is improved, at the cost of more calls to malloc. The stats after the above change are: numMallocs: 6458 numReallocs: 16 numFrees: 6458 hwmRequested: 323864 hwmReal: 324352 The histogram shows that the change has had its intended effect: (256,256),380 (300,304),1 (668,1024),1 (1024,1024),5909 (1048,1536),18 (2072,2560),5 (8192,8192),37 (12288,12288),123 This is a further reduction of real memory use (by a further 5%) over the prior patch. The cumulative effect of this change, bug 415967 and bug 420476, is a 14% reduction in real memory use by jsarenas, in the 'start-stop' test. In my informal 'use Gmail' test, this patch made the results slightly worse than with only 415967 applied. This makes sense, because it means that larger arenas (than requested) are doing the code some good. I will open a low priority defect to tune the arena sizes.
Keywords: footprint, perf
(In reply to comment #4) > I will open a low priority defect to tune the arena sizes. It's bug 421435.
Attached patch v2, relative to patch from 415967 (obsolete) (deleted) — Splinter Review
Minor improvements to prior patch.
Attachment #307817 - Attachment is obsolete: true
Attachment #309173 - Flags: review?(crowder)
Depends on: 415967
I'll make a patch which does not depend on 415967.
No longer depends on: 415967
Attached patch v3, the patch is now independent of 415967 (obsolete) (deleted) — Splinter Review
... as promised. I continue to investigate performance characteristics of this patch.
Attachment #309173 - Attachment is obsolete: true
Attachment #309173 - Flags: review?(crowder)
Depends on: 412866
Blocks: 421435
Attached patch v4 - on top of 412866, 424287 (deleted) — Splinter Review
This is an updated patch which includes changes from bug 412866 and 424287. (See bug 408921 for the motivation for this bug.)
Attachment #310721 - Attachment is obsolete: true
Depends on: 424287
Blocks: 408921
Bug 675545 will fix this by only allowing alignment to be one of 1, 2, 4 or 8, which makes everything else much simpler.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: