Closed
Bug 481788
Opened 16 years ago
Closed 16 years ago
[FIX]Switch FrameConstructionItemList to using PRCList
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(1 file, 1 obsolete file)
8.14 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
And probably make it a linked list in the process.
Assignee | ||
Comment 1•16 years ago
|
||
OK, so I definitely want to make this a linked list. As far as arena allocation goes, Stuart says that fragmentation is not a worry: in the size I'm talking about here (56 bytes for a FrameConstructionItem at the moment, but likely to grow by a few words) jemalloc effectively has a built-in arena. I did some measurements to see how many FrameConstructionItems we're allocating and what the high water marks are for various tasks.
Task | Allocations | High water mark
--------------------------------------------------------------
Browser startup | 200 | 35
Type "cnn.com" in url bar | 358 | 35
Load cnn.com | ~3000 | 66
Load html5 single-page | ~175,000 | 1019
Pretty-print 700KB XML file | ~150,000 | 2421
So we would be recycling objects off that list all along, basically. So the only reason to use an arena-like thing would be performance, right? jemalloc does apparently do some locking, so it might be nice to avoid that.
If we do want to do an arena, then it seems like an nsFixedSizeAllocator is the way to go. That's what we use elsewhere (for nodeinfos, parser stuff, etc). Given the above high-water marks, it seems like using a static fixed-size allocator that we don't bother to clear (as we do for other cases) should not be a problem: 2500 * 56 bytes == 75KB memory usage.
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #366324 -
Flags: superreview?(roc)
Attachment #366324 -
Flags: review?(roc)
In comment #1 you pretty much convinced me we don't need the custom arena :-). jemalloc or any other good allocator should have a thread-local freelist so some number of objects can be allocated and freed without locking. Feel like creating a microbenchmark in nsCSSFrameConstructor to compare a million allocations and releases of a hundred objects each way?
Otherwise, if you want to go with what you've got, it looks fine, except it seems to me there is no reason not to release the pool at the end of a frame construction phase --- that should be super-easy and simple right?
Assignee | ||
Comment 4•16 years ago
|
||
OK, did the microbenchmark on Mac (so no jemalloc; building on Linux now to test jemalloc).
The upshot is that the fixed-size allocator handles that test in about 40ns per alloc/free pair, while the default OS allocator needs more like 240ns. Given the numbers above, that comes out to 6ms vs 36ms allocating all the FrameConstructionItems on that 700KB XML file, or 7ms vs 42ms on the HTML5 load. Not worth worrying about.
So if fragmentation is not a worry (including on mac, where we don't use jemalloc), code simplicity says no custom allocator stuff here.
Assignee | ||
Comment 5•16 years ago
|
||
On Linux, I get similar numbers: 20s for jemalloc, 5s for the fixed size allocator (the Mac numbers above correspond to 24s for system malloc, 4s for the fixed size allocator).
Still not worth worrying about.
Assignee | ||
Comment 6•16 years ago
|
||
Hijacking my own bug.
Summary: Switch FrameConstructionItemList to arena allocation → [FIX]Switch FrameConstructionItemList to using PRCList
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #366324 -
Attachment is obsolete: true
Attachment #366481 -
Flags: superreview?(roc)
Attachment #366481 -
Flags: review?(roc)
Attachment #366324 -
Flags: superreview?(roc)
Attachment #366324 -
Flags: review?(roc)
Attachment #366481 -
Flags: superreview?(roc)
Attachment #366481 -
Flags: superreview+
Attachment #366481 -
Flags: review?(roc)
Attachment #366481 -
Flags: review+
Assignee | ||
Comment 8•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•