Switch nsRegionAllocator to ThreadLocal and avoid extra library call

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
mozilla15
ARM
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Possibly similar to 756439, I think it make sense to switch Regions to ThreadLocal and avoid nspr calls for multiple region operations
(Assignee)

Comment 1

5 years ago
Created attachment 626529 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #626529 - Flags: review?(bgirard)
Comment on attachment 626529 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call

Cehcking with Ehsan to make sure we're good with using this throughout the code base instead of nspr.
Attachment #626529 - Flags: review?(ehsan)
Attachment #626529 - Flags: review?(bgirard)
Attachment #626529 - Flags: review+
Comment on attachment 626529 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call

Review of attachment 626529 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the static_casts below removed.

::: gfx/src/nsRegion.cpp
@@ +188,5 @@
>  
>  void RgnRectMemoryAllocatorDTOR(void *priv)
>  {
>    RgnRectMemoryAllocator* allocator = (static_cast<RgnRectMemoryAllocator*>(
> +                                       gRectPoolTlsIndex.get()));

You should not need the cast here and below.
Attachment #626529 - Flags: review?(ehsan) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 626540 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call

Static cast removed, and added shutdown
Attachment #626529 - Attachment is obsolete: true
Attachment #626540 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 703317
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f93e4add42b
Flags: in-testsuite-
Target Milestone: --- → mozilla15
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ec7e8407e5 - either this or you patch for bug 757380 was hitting mochitest-4 assertions like https://tbpl.mozilla.org/php/getParsedLog.php?id=12048582&tree=Mozilla-Inbound&full=1
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5baac057fc1a
Keywords: checkin-needed
And backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/be42b12c6092

https://tbpl.mozilla.org/php/getParsedLog.php?id=12096646&tree=Mozilla-Inbound

../../../gfx/src/nsRegion.cpp: In function 'void RgnRectMemoryAllocatorDTOR(void*)':
../../../gfx/src/nsRegion.cpp:191:61: error: cannot convert 'RgnRectMemoryAllocator' to 'RgnRectMemoryAllocator*' in initialization
../../../gfx/src/nsRegion.cpp: In static member function 'static void nsRegion::ShutdownStatic()':
../../../gfx/src/nsRegion.cpp:202:61: error: cannot convert 'RgnRectMemoryAllocator' to 'RgnRectMemoryAllocator*' in initialization
../../../gfx/src/nsRegion.cpp: In static member function 'static void* nsRegion::RgnRect::operator new(size_t)':
../../../gfx/src/nsRegion.cpp:213:61: error: cannot convert 'RgnRectMemoryAllocator' to 'RgnRectMemoryAllocator*' in initialization
../../../gfx/src/nsRegion.cpp:216:36: error: no matching function for call to 'mozilla::ThreadLocal<RgnRectMemoryAllocator>::set(RgnRectMemoryAllocator*&)'
../../dist/include/mozilla/ThreadLocal.h:125:1: note: candidate is: bool mozilla::ThreadLocal<T>::set(T) [with T = RgnRectMemoryAllocator]
../../../gfx/src/nsRegion.cpp: In static member function 'static void nsRegion::RgnRect::operator delete(void*, size_t)':
../../../gfx/src/nsRegion.cpp:223:61: error: cannot convert 'RgnRectMemoryAllocator' to 'RgnRectMemoryAllocator*' in initialization
In file included from ../../../gfx/src/nsRegion.cpp:8:0:
../../dist/include/mozilla/ThreadLocal.h: At global scope:
../../dist/include/mozilla/ThreadLocal.h: In instantiation of 'mozilla::ThreadLocal<RgnRectMemoryAllocator>::Helper':
../../dist/include/mozilla/ThreadLocal.h:114:10:   instantiated from 'T mozilla::ThreadLocal<T>::get() const [with T = RgnRectMemoryAllocator]'
../../../gfx/src/nsRegion.cpp:191:61:   instantiated from here
../../dist/include/mozilla/ThreadLocal.h:77:9: error: member 'RgnRectMemoryAllocator mozilla::ThreadLocal<RgnRectMemoryAllocator>::Helper::value' with constructor not allowed in union
../../dist/include/mozilla/ThreadLocal.h:77:9: error: member 'RgnRectMemoryAllocator mozilla::ThreadLocal<RgnRectMemoryAllocator>::Helper::value' with destructor not allowed in union
../../dist/include/mozilla/ThreadLocal.h: In member function 'bool mozilla::ThreadLocal<T>::init() [with T = RgnRectMemoryAllocator]':
../../../gfx/src/nsRegion.cpp:197:33:   instantiated from here
../../dist/include/mozilla/ThreadLocal.h:99:3: error: static assertion failed: "mozilla::ThreadLocal can\'t be used for types larger than a pointer"
Target Milestone: mozilla15 → ---
(Assignee)

Comment 9

5 years ago
Created attachment 627488 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call

After bug  756965 landed, patch need update
-mozilla::ThreadLocal<RgnRectMemoryAllocator> gRectPoolTlsIndex;
+mozilla::ThreadLocal<RgnRectMemoryAllocator*> gRectPoolTlsIndex;
Here is try build for last patch 
https://tbpl.mozilla.org/?tree=Try&rev=c30e13ce681c
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c3bcd58f3b
Target Milestone: --- → mozilla15
Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/5baac057fc1a
https://hg.mozilla.org/mozilla-central/rev/be42b12c6092

Relanded:
https://hg.mozilla.org/mozilla-central/rev/34c3bcd58f3b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.