Closed Bug 420476 Opened 12 years ago Closed 9 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) — 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.
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. 

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
... 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
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: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 675545
You need to log in before you can comment on or make changes to this bug.