679191 - Improve XPT Arena size increasing behavior

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

PT 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 [inactive]]

Do you have data that this is needed?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 [inactive]]

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 [inactive]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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 [inactive]]

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 (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/1248e3a8a8c8