757933 - Switch nsRegionAllocator to ThreadLocal and avoid extra library call

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Possibly similar to 756439, I think it make sense to switch Regions to ThreadLocal and avoid nspr calls for multiple region operations

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: NSPR TLS to ThreadLocal fast call

Comment 2

[Benoit Girard (:BenWa)]

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.

Comment 3

[(no longer active)]

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.

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: NSPR TLS to ThreadLocal fast call

Static cast removed, and added shutdown

Comment 5

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f93e4add42b

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5baac057fc1a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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"

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: 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

Comment 10

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34c3bcd58f3b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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