Improve XPT Arena size increasing behavior

RESOLVED FIXED in mozilla8

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla8
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

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.
Attachment #553297 - Flags: review?(nnethercote)
Do you have data that this is needed?
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 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.
Attachment #553297 - Flags: review?(nnethercote) → review+
(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 :)
Whiteboard: [MemShrink]
(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.
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.
http://hg.mozilla.org/mozilla-central/rev/1248e3a8a8c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.