Last Comment Bug 757933 - Switch nsRegionAllocator to ThreadLocal and avoid extra library call
: Switch nsRegionAllocator to ThreadLocal and avoid extra library call
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla15
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks: 703317
  Show dependency treegraph
 
Reported: 2012-05-23 11:36 PDT by Oleg Romashin (:romaxa)
Modified: 2012-05-26 15:29 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
NSPR TLS to ThreadLocal fast call (3.06 KB, patch)
2012-05-23 11:38 PDT, Oleg Romashin (:romaxa)
b56girard: review+
ehsan: review+
Details | Diff | Splinter Review
NSPR TLS to ThreadLocal fast call (3.08 KB, patch)
2012-05-23 12:06 PDT, Oleg Romashin (:romaxa)
romaxa: review+
Details | Diff | Splinter Review
NSPR TLS to ThreadLocal fast call (3.16 KB, patch)
2012-05-26 09:05 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2012-05-23 11:36:01 PDT
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) 2012-05-23 11:38:31 PDT
Created attachment 626529 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call
Comment 2 Benoit Girard (:BenWa) 2012-05-23 11:42:30 PDT
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 :Ehsan Akhgari 2012-05-23 11:54:11 PDT
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) 2012-05-23 12:06:47 PDT
Created attachment 626540 [details] [diff] [review]
NSPR TLS to ThreadLocal fast call

Static cast removed, and added shutdown
Comment 6 Phil Ringnalda (:philor) 2012-05-24 19:58:20 PDT
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] 2012-05-26 06:41:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5baac057fc1a
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-05-26 07:11:37 PDT
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) 2012-05-26 09:05:48 PDT
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

Note You need to log in before you can comment on or make changes to this bug.