Last Comment Bug 679191 - Improve XPT Arena size increasing behavior
: Improve XPT Arena size increasing behavior
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-15 16:13 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2011-08-16 04:28 PDT (History)
2 users (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (914 bytes, patch)
2011-08-15 16:13 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
n.nethercote: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-15 16:13:03 PDT
Created attachment 553297 [details] [diff] [review]
Patch

XPT arenas behave well, unless you ask them for a single allocation larger than the block size.  In that case, they allocate uneven blocks.  This is probably a micro-optimization, but it's easy enough to do.
Comment 1 Nicholas Nethercote [:njn] 2011-08-15 18:16:22 PDT
Do you have data that this is needed?
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-15 18:59:30 PDT
Depends on how you define "needed".  My ad-hoc instrumentation showed that it looked like we're wasting about 10% of the memory allocated here.  Using a total of 1 MB from the explicit/xpti-working-set reporter we're looking at 100 KB wasted for the lifetime of the browser.  Not a big contribution, but given that it's a two line fix ...
Comment 3 Nicholas Nethercote [:njn] 2011-08-15 19:07:55 PDT
Comment on attachment 553297 [details] [diff] [review]
Patch

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

::: xpcom/typelib/xpt/src/xpt_arena.c
@@ +215,5 @@
>          size_t block_header_size = ALIGN_RND(sizeof(BLK_HDR), arena->alignment);
>          size_t new_space = arena->block_size;
>           
> +        while (bytes > new_space - block_header_size)
> +            new_space += arena->block_size;

A comment here would be good, explaining that block_size is a power-of-two (I hope that's correct) and so this avoids slop.
Comment 4 Nicholas Nethercote [:njn] 2011-08-15 19:08:51 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Depends on how you define "needed".  My ad-hoc instrumentation showed that
> it looked like we're wasting about 10% of the memory allocated here.  Using
> a total of 1 MB from the explicit/xpti-working-set reporter we're looking at
> 100 KB wasted for the lifetime of the browser.  Not a big contribution, but
> given that it's a two line fix ...

Ok, that's very reasonable.  If you'd included that info in the patch submission I wouldn't need to have asked :)
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-15 19:10:54 PDT
(In reply to Nicholas Nethercote [:njn] from comment #4)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> > Depends on how you define "needed".  My ad-hoc instrumentation showed that
> > it looked like we're wasting about 10% of the memory allocated here.  Using
> > a total of 1 MB from the explicit/xpti-working-set reporter we're looking at
> > 100 KB wasted for the lifetime of the browser.  Not a big contribution, but
> > given that it's a two line fix ...
> 
> Ok, that's very reasonable.  If you'd included that info in the patch
> submission I wouldn't need to have asked :)

Well, I didn't measure until you asked ;-).  I was just reading the code because I expected it had the same type of fail as PLArena and noticed this.
Comment 6 Nicholas Nethercote [:njn] 2011-08-15 19:12:40 PDT
PLArena (and JSArena) have the same issue on oversized allocations but I don't plan to do anything about it until profiling tells me it's important.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-16 04:28:52 PDT
http://hg.mozilla.org/mozilla-central/rev/1248e3a8a8c8

Note You need to log in before you can comment on or make changes to this bug.