Closed Bug 703317 Opened 8 years ago Closed 8 years ago

non-threadsafe allocator for nsIntRegion causes TestDataStructures to fail

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: nmatsakis, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

The test TestDataStructures uses nsIntRegions which are allocated via a non-threadsafe allocator.  The result is that the Test18() subroutine is not usable in a cross-thread context.  Presumably nsIntRegions are also used in other contexts which will also not be usable in a cross-thread context.  Making the allocator thread-local rather than global might be a possible solution.
Depends on: 699319
This is one of the bugs blocking off-main-thread compositing.

I don't have a good feel for how much this optimized allocator helps us.  My first inclination would be to try an implementation using malloc/free and see if anything regresses.
Component: IPC → Graphics
OS: Mac OS X → All
QA Contact: ipc → thebes
Hardware: x86 → All
We create and destroy regions *a lot*. I expect removing this would regress something. Maybe I'm wrong though.

So, TLS recycler?
I'm looking at the allocator and I find this very concerning:
http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRegion.cpp#99

This means to indicate we can have an adversarial case where a page causes a lot of region allocation. Closing the tab wouldn't release the memory.

I see 3 solutions:
1) TLSing the allocator
2) Use malloc
3) Add MUTEX for the allocator

Should we investigate TLSing first? Solution 2 should fix the memory poll release problem if it doesn't regress performance.
Try run for 20ae1971dd5d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=20ae1971dd5d
Results (out of 6 total builds):
    exception: 4
    success: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-20ae1971dd5d
(In reply to Benoit Girard (:BenWa) from comment #3)
> I'm looking at the allocator and I find this very concerning:
> http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRegion.cpp#99
> 
> This means to indicate we can have an adversarial case where a page causes a
> lot of region allocation. Closing the tab wouldn't release the memory.
> 
> I see 3 solutions:
> 1) TLSing the allocator
> 2) Use malloc
> 3) Add MUTEX for the allocator

A good option might be TLS plus time-based expiration of the rect pool.

Maybe someone should just write a region micro-benchmark that creates and destroys a lot of regions with 1 and 2 rectangles in them, then see if there's a measurable speedup from the free rect pool. Maybe jemalloc optimizes these 16-byte allocations really well.
Try run for d00b1a3dcccf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d00b1a3dcccf
Results (out of 49 total builds):
    success: 48
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-d00b1a3dcccf
I got a 2% tp4 regression on OSX/XP/Win7 so I think option 2 is out of the question.
Attached patch TLSAllocator v1Splinter Review
Pushed to try
Blocks: 699319
No longer depends on: 699319
Try run for 0b82febeb6ea is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0b82febeb6ea
Results (out of 104 total builds):
    success: 89
    warnings: 14
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0b82febeb6ea
Comment on attachment 577667 [details] [diff] [review]
TLSAllocator v1

Good try run, worked well locally.
Attachment #577667 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/47500d6cbffd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
No longer blocks: omtc
Blocks: omtc
I've been playing recently with patches from bug 539356, and by default without last patches it introduces significant amount of region operations. and in couple with this fix I see significant performance hit caused by threadsafety checks on page which introduce lot of regions ops http://bubblemark.com/dhtml.htm
is there are anything we can do about it?
Do we have profiles to show what is slow?
Yeah, which threadsafety checks are you referring to?
Access to per-thread rectangle memory pool.
Depends on: 757933
OK, TLS access is different than thread-safety checks.  I defer to comment 14 then.
http://pastebin.mozilla.org/1648703 - here is opreport with fixed 757933, nspr calls are gone, bug pthread_getspecific still on top of profile
Yay ARM.  Definitely want to be __thread'ing this.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> Yay ARM.  Definitely want to be __thread'ing this.

Filed bug 757969 on that.
Depends on: 774756
You need to log in before you can comment on or make changes to this bug.