non-threadsafe allocator for nsIntRegion causes TestDataStructures to fail

RESOLVED FIXED in mozilla11

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: nmatsakis, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
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
Blocks: 598873
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.

Comment 4

6 years ago
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.

Comment 6

6 years ago
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.
That's good to know :-)
Created attachment 577667 [details] [diff] [review]
TLSAllocator v1

Pushed to try

Updated

6 years ago
Blocks: 699319
No longer depends on: 699319

Comment 10

6 years ago
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)
Attachment #577667 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/47500d6cbffd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
No longer blocks: 598873
Blocks: 598873
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.
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.

Updated

5 years ago
Depends on: 774756
You need to log in before you can comment on or make changes to this bug.