Closed Bug 703317 Opened 8 years ago Closed 8 years ago
non-threadsafe allocator for ns
Int Region causes Test Data Structures to fail
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.
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://firstname.lastname@example.org
(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://email@example.com
I got a 2% tp4 regression on OSX/XP/Win7 so I think option 2 is out of the question.
That's good to know :-)
Pushed to try
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://firstname.lastname@example.org
Comment on attachment 577667 [details] [diff] [review] TLSAllocator v1 Good try run, worked well locally.
Attachment #577667 - Flags: review?(roc)
Attachment #577667 - Flags: review?(roc) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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.
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.
Thanks for the fix!
(In reply to Chris Jones [:cjones] [:warhammer] from comment #19) > Yay ARM. Definitely want to be __thread'ing this. Filed bug 757969 on that.
You need to log in before you can comment on or make changes to this bug.