Windows now uses the TLS API instead of __declspec(thread) for client side co...

Windows now uses the TLS API instead of __declspec(thread) for client side command buffer code compiled into DLLs. Other platforms use the pthreads API. This is because the __declspec(thread) approach does not on some platforms, including  Windows XP and Mac.

This is used for thread local pointers to the GL and PGL contexts. This unfortunate because the PGL and GL APIs do not generally explicitly reference a context. The current context is set with a call to pglMakeCurrent.

An unfortunate consequence is that now in Pepper plugins, every call to a GL function will call TlsGetValue to get the thread's current context, which could have performance issues.

I can't use base::ThreadLocalPointer because this code is compiled into an untrusted NaCl module and we don't want Chromium dependencies.

TEST=try
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=37300

[alokp, 2010-01-22 00:40:05]

LGTM

A couple of things:
1. Should you add a TODO to remove this workaround sometime in the future?
2. I am just a bit concerned about the fact that we keep copying code from base.
Not all of base is problematic under NaCl. Last time I checked it was just the
logging stuff that has already been stubbed out in gpu. I think fixing the base
code is more scalable as we port more stuff to NaCl.

[apatrick_chromium, 2010-01-22 19:50:33]

cpu, any thoughts before I commit this atrocity? See description for the issue.

[apatrick_chromium, 2010-01-22 23:57:55]

I reimplemented TlsGetValue and TlsSetValue in inline assembly. Somebody stop
me!

[cpu_(ooo_6.6-7.5), 2010-01-23 00:19:11]

Wow sweet baby jesus, gimme a day to digest this!


On 2010/01/22 23:57:55, apatrick_chromium wrote:
> I reimplemented TlsGetValue and TlsSetValue in inline assembly. Somebody stop
> me!

[cpu_(ooo_6.6-7.5), 2010-01-23 02:12:57]

http://codereview.chromium.org/553050/diff/7002/8008
File gpu/command_buffer/client/gles2_lib.h (right):

http://codereview.chromium.org/553050/diff/7002/8008#newcode15
gpu/command_buffer/client/gles2_lib.h:15: extern THREAD_LOCAL
gpu::ThreadLocalPointer<gpu::gles2::GLES2Implementation>
I am soo confused about this, it looks like THREAD_LOCAL becomes "" given the
current defines, but if the defines change then THREAD_LOCAL becomes
__declspec(thread)

If so the expression in line 15 makes no sense to me.

It seems to me that you should stop the macro insanity and have one single Tls
implementation that works in all cases.

http://codereview.chromium.org/553050/diff/7002/8006
File gpu/command_buffer/common/thread_local_pointer.h (right):

http://codereview.chromium.org/553050/diff/7002/8006#newcode69
gpu/command_buffer/common/thread_local_pointer.h:69: void* Get() const {
if your aim here was to save the overhead of a function call over TlsGetValue,
It might not happen

http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx

It says there than functions with inline assembly might not get inlined and thus
the benefit is lost.

http://codereview.chromium.org/553050/diff/7002/8006#newcode82
gpu/command_buffer/common/thread_local_pointer.h:82: 
why does set need to be assembly? Is it done all the time as well?

http://codereview.chromium.org/553050/diff/7002/8006#newcode134
gpu/command_buffer/common/thread_local_pointer.h:134: UntypedThreadLocalPointer
pointer_;
shouldn't this be private?

[apatrick, 2010-01-23 02:30:01]

It's a mess right now. I'm trying to explore the options. I wish
__declspec(thread) would just work but the docs say it doesn't work for
dynamically loaded DLLs until Vista. I'm not so concerned about Get being
inlined. I'm uncomfortable about making a system call for every GL call and
I want to know if there are viable alternatives that I can compare
performance with. I've read reports on the web that there are performance
issues with frequent TlsGetValue calls. If a function is in kernel32.dll,
does that mean it switches into kernel land? Set doesn't need to be
implemented in assembly.

Is there maybe an intrinsic to get at the FS register?

On Fri, Jan 22, 2010 at 6:12 PM, <cpu@chromium.org> wrote:

>
> http://codereview.chromium.org/553050/diff/7002/8008
> File gpu/command_buffer/client/gles2_lib.h (right):
>
> http://codereview.chromium.org/553050/diff/7002/8008#newcode15
> gpu/command_buffer/client/gles2_lib.h:15: extern THREAD_LOCAL
> gpu::ThreadLocalPointer<gpu::gles2::GLES2Implementation>
> I am soo confused about this, it looks like THREAD_LOCAL becomes ""
> given the current defines, but if the defines change then THREAD_LOCAL
> becomes __declspec(thread)
>
> If so the expression in line 15 makes no sense to me.
>
> It seems to me that you should stop the macro insanity and have one
> single Tls implementation that works in all cases.
>
> http://codereview.chromium.org/553050/diff/7002/8006
> File gpu/command_buffer/common/thread_local_pointer.h (right):
>
> http://codereview.chromium.org/553050/diff/7002/8006#newcode69
> gpu/command_buffer/common/thread_local_pointer.h:69: void* Get() const {
> if your aim here was to save the overhead of a function call over
> TlsGetValue, It might not happen
>
> http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
>
> It says there than functions with inline assembly might not get inlined
> and thus the benefit is lost.
>
> http://codereview.chromium.org/553050/diff/7002/8006#newcode82
> gpu/command_buffer/common/thread_local_pointer.h:82:
> why does set need to be assembly? Is it done all the time as well?
>
> http://codereview.chromium.org/553050/diff/7002/8006#newcode134
> gpu/command_buffer/common/thread_local_pointer.h:134:
> UntypedThreadLocalPointer pointer_;
> shouldn't this be private?
>
>
> http://codereview.chromium.org/553050
>

[apatrick, 2010-01-23 02:45:33]

Oh no :( There is:

unsigned long __readfsdword(
   unsigned long Offset
);

http://msdn.microsoft.com/en-us/library/3887zk1s(VS.80).aspx

And the TEB structure along with the TLS array offset is documented here:

http://msdn.microsoft.com/en-us/library/ms686708(VS.85).aspx

<http://msdn.microsoft.com/en-us/library/ms686708(VS.85).aspx>I hate myself.

On Fri, Jan 22, 2010 at 6:29 PM, Alastair Patrick <apatrick@google.com>wrote:

> It's a mess right now. I'm trying to explore the options. I wish
> __declspec(thread) would just work but the docs say it doesn't work for
> dynamically loaded DLLs until Vista. I'm not so concerned about Get being
> inlined. I'm uncomfortable about making a system call for every GL call and
> I want to know if there are viable alternatives that I can compare
> performance with. I've read reports on the web that there are performance
> issues with frequent TlsGetValue calls. If a function is in kernel32.dll,
> does that mean it switches into kernel land? Set doesn't need to be
> implemented in assembly.
>
> Is there maybe an intrinsic to get at the FS register?
>
>
> On Fri, Jan 22, 2010 at 6:12 PM, <cpu@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/553050/diff/7002/8008
>> File gpu/command_buffer/client/gles2_lib.h (right):
>>
>> http://codereview.chromium.org/553050/diff/7002/8008#newcode15
>> gpu/command_buffer/client/gles2_lib.h:15: extern THREAD_LOCAL
>> gpu::ThreadLocalPointer<gpu::gles2::GLES2Implementation>
>> I am soo confused about this, it looks like THREAD_LOCAL becomes ""
>> given the current defines, but if the defines change then THREAD_LOCAL
>> becomes __declspec(thread)
>>
>> If so the expression in line 15 makes no sense to me.
>>
>> It seems to me that you should stop the macro insanity and have one
>> single Tls implementation that works in all cases.
>>
>> http://codereview.chromium.org/553050/diff/7002/8006
>> File gpu/command_buffer/common/thread_local_pointer.h (right):
>>
>> http://codereview.chromium.org/553050/diff/7002/8006#newcode69
>> gpu/command_buffer/common/thread_local_pointer.h:69: void* Get() const {
>> if your aim here was to save the overhead of a function call over
>> TlsGetValue, It might not happen
>>
>> http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
>>
>> It says there than functions with inline assembly might not get inlined
>> and thus the benefit is lost.
>>
>> http://codereview.chromium.org/553050/diff/7002/8006#newcode82
>> gpu/command_buffer/common/thread_local_pointer.h:82:
>> why does set need to be assembly? Is it done all the time as well?
>>
>> http://codereview.chromium.org/553050/diff/7002/8006#newcode134
>> gpu/command_buffer/common/thread_local_pointer.h:134:
>> UntypedThreadLocalPointer pointer_;
>> shouldn't this be private?
>>
>>
>> http://codereview.chromium.org/553050
>>
>
>

[rvargas (doing something else), 2010-01-23 03:26:20]

On 2010/01/23 02:45:33, apatrick wrote:
> Oh no :( There is:
> 
> unsigned long __readfsdword(
>    unsigned long Offset
> );
> 
> http://msdn.microsoft.com/en-us/library/3887zk1s%28VS.80%29.aspx
> 
> And the TEB structure along with the TLS array offset is documented here:
> 
> http://msdn.microsoft.com/en-us/library/ms686708%28VS.85%29.aspx
> 
> <http://msdn.microsoft.com/en-us/library/ms686708%28VS.85%29.aspx%3EI hate
myself.
> 
> On Fri, Jan 22, 2010 at 6:29 PM, Alastair Patrick <apatrick@google.com>wrote:
> 
> > It's a mess right now. I'm trying to explore the options. I wish
> > __declspec(thread) would just work but the docs say it doesn't work for
> > dynamically loaded DLLs until Vista. I'm not so concerned about Get being
> > inlined. I'm uncomfortable about making a system call for every GL call and
> > I want to know if there are viable alternatives that I can compare
> > performance with. I've read reports on the web that there are performance
> > issues with frequent TlsGetValue calls. If a function is in kernel32.dll,
> > does that mean it switches into kernel land? Set doesn't need to be
> > implemented in assembly.

The code of kernel32!TlsGetValue basically does what you are doing except that:

it sets last_error to 0
verifies that the tls_index is under the size of the array.


> >
> > Is there maybe an intrinsic to get at the FS register?
> >
> >
> > On Fri, Jan 22, 2010 at 6:12 PM, <mailto:cpu@chromium.org> wrote:
> >
> >>
> >> http://codereview.chromium.org/553050/diff/7002/8008
> >> File gpu/command_buffer/client/gles2_lib.h (right):
> >>
> >> http://codereview.chromium.org/553050/diff/7002/8008#newcode15
> >> gpu/command_buffer/client/gles2_lib.h:15: extern THREAD_LOCAL
> >> gpu::ThreadLocalPointer<gpu::gles2::GLES2Implementation>
> >> I am soo confused about this, it looks like THREAD_LOCAL becomes ""
> >> given the current defines, but if the defines change then THREAD_LOCAL
> >> becomes __declspec(thread)
> >>
> >> If so the expression in line 15 makes no sense to me.
> >>
> >> It seems to me that you should stop the macro insanity and have one
> >> single Tls implementation that works in all cases.
> >>
> >> http://codereview.chromium.org/553050/diff/7002/8006
> >> File gpu/command_buffer/common/thread_local_pointer.h (right):
> >>
> >> http://codereview.chromium.org/553050/diff/7002/8006#newcode69
> >> gpu/command_buffer/common/thread_local_pointer.h:69: void* Get() const {
> >> if your aim here was to save the overhead of a function call over
> >> TlsGetValue, It might not happen
> >>
> >> http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
> >>
> >> It says there than functions with inline assembly might not get inlined
> >> and thus the benefit is lost.
> >>
> >> http://codereview.chromium.org/553050/diff/7002/8006#newcode82
> >> gpu/command_buffer/common/thread_local_pointer.h:82:
> >> why does set need to be assembly? Is it done all the time as well?
> >>
> >> http://codereview.chromium.org/553050/diff/7002/8006#newcode134
> >> gpu/command_buffer/common/thread_local_pointer.h:134:
> >> UntypedThreadLocalPointer pointer_;
> >> shouldn't this be private?
> >>
> >>
> >> http://codereview.chromium.org/553050
> >>
> >
> >
>

[cpu_(ooo_6.6-7.5), 2010-01-25 20:32:12]

As Rick confirmed the overhead of TlsGetValue() is near zero.


On 2010/01/23 03:26:20, rvargas wrote:
> On 2010/01/23 02:45:33, apatrick wrote:
> > Oh no :( There is:
> > 
> > unsigned long __readfsdword(
> >    unsigned long Offset
> > );
> > 
> > http://msdn.microsoft.com/en-us/library/3887zk1s%2528VS.80%2529.aspx
> > 
> > And the TEB structure along with the TLS array offset is documented here:
> > 
> > http://msdn.microsoft.com/en-us/library/ms686708%2528VS.85%2529.aspx
> > 
> > <http://msdn.microsoft.com/en-us/library/ms686708%2528VS.85%2529.aspx%253EI
hate
> myself.
> > 
> > On Fri, Jan 22, 2010 at 6:29 PM, Alastair Patrick
<apatrick@google.com>wrote:
> > 
> > > It's a mess right now. I'm trying to explore the options. I wish
> > > __declspec(thread) would just work but the docs say it doesn't work for
> > > dynamically loaded DLLs until Vista. I'm not so concerned about Get being
> > > inlined. I'm uncomfortable about making a system call for every GL call
and
> > > I want to know if there are viable alternatives that I can compare
> > > performance with. I've read reports on the web that there are performance
> > > issues with frequent TlsGetValue calls. If a function is in kernel32.dll,
> > > does that mean it switches into kernel land? Set doesn't need to be
> > > implemented in assembly.
> 
> The code of kernel32!TlsGetValue basically does what you are doing except
that:
> 
> it sets last_error to 0
> verifies that the tls_index is under the size of the array.
> 
> 
> > >
> > > Is there maybe an intrinsic to get at the FS register?
> > >
> > >
> > > On Fri, Jan 22, 2010 at 6:12 PM, <mailto:cpu@chromium.org> wrote:
> > >
> > >>
> > >> http://codereview.chromium.org/553050/diff/7002/8008
> > >> File gpu/command_buffer/client/gles2_lib.h (right):
> > >>
> > >> http://codereview.chromium.org/553050/diff/7002/8008#newcode15
> > >> gpu/command_buffer/client/gles2_lib.h:15: extern THREAD_LOCAL
> > >> gpu::ThreadLocalPointer<gpu::gles2::GLES2Implementation>
> > >> I am soo confused about this, it looks like THREAD_LOCAL becomes ""
> > >> given the current defines, but if the defines change then THREAD_LOCAL
> > >> becomes __declspec(thread)
> > >>
> > >> If so the expression in line 15 makes no sense to me.
> > >>
> > >> It seems to me that you should stop the macro insanity and have one
> > >> single Tls implementation that works in all cases.
> > >>
> > >> http://codereview.chromium.org/553050/diff/7002/8006
> > >> File gpu/command_buffer/common/thread_local_pointer.h (right):
> > >>
> > >> http://codereview.chromium.org/553050/diff/7002/8006#newcode69
> > >> gpu/command_buffer/common/thread_local_pointer.h:69: void* Get() const {
> > >> if your aim here was to save the overhead of a function call over
> > >> TlsGetValue, It might not happen
> > >>
> > >> http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx
> > >>
> > >> It says there than functions with inline assembly might not get inlined
> > >> and thus the benefit is lost.
> > >>
> > >> http://codereview.chromium.org/553050/diff/7002/8006#newcode82
> > >> gpu/command_buffer/common/thread_local_pointer.h:82:
> > >> why does set need to be assembly? Is it done all the time as well?
> > >>
> > >> http://codereview.chromium.org/553050/diff/7002/8006#newcode134
> > >> gpu/command_buffer/common/thread_local_pointer.h:134:
> > >> UntypedThreadLocalPointer pointer_;
> > >> shouldn't this be private?
> > >>
> > >>
> > >> http://codereview.chromium.org/553050
> > >>
> > >
> > >
> >

[apatrick_chromium, 2010-01-26 22:14:47]

I compared the performance of TlsGetValue against my own implementation. A tight
loop calling TlsGetValue repeatedly only took about twice as long on XP and
64-bit Vista. I think that should be okay. I don't know about the performance of
pthread_getspecific but the docs claim it should be designed to be quick.

I'll leave this here in case I need it again and so you can all laugh at it.

void* MyTlsGetValue(DWORD index) {
  return reinterpret_cast<void*>(
    __readfsdword(offsetof(TEB, TlsSlots) + index * sizeof(void*)));
}

Anyway, it isn't doing anything insane anymore. Please review.

[cpu_(ooo_6.6-7.5), 2010-01-27 00:26:13]

lgtm on the TLS stuff. I did not check the rest I assume the others reviewers
will do.

[alokp, 2010-01-27 05:30:19]

lgtm

http://codereview.chromium.org/553050/diff/47/1037
File gpu/demos/framework/plugin.cc (right):

http://codereview.chromium.org/553050/diff/47/1037#newcode97
gpu/demos/framework/plugin.cc:97: demo_.reset();
may be demo_.release() is clearer.

[darin (slow to review), 2010-01-27 07:36:30]

Is there some reason why base/thread_local.h was not used?  Are we trying to
avoid a dependency on base?

[apatrick, 2010-01-27 07:56:15]

Yes I am avoiding a dependency on base. This will be built as a NaCl
untrusted library and linked with NaCl modules so they can use OpenGL on top
of the NPDevice3D interface. The prevailing idea is that this should not be
dependent on base. I am beginning to question it.

[rvargas (doing something else), 2010-01-27 19:45:17]

http://codereview.chromium.org/553050/diff/47/1029
File gpu/pgl/pgl.cc (right):

http://codereview.chromium.org/553050/diff/47/1029#newcode147
gpu/pgl/pgl.cc:147: PGLContext pglCreateContext(NPP npp,
Out of curiosity, what's the reason for having this code not following our
coding style?. I mean, everything else looks like our style, but this naming
style is definitely foreign.

[apatrick_chromium, 2010-01-27 19:55:09]

http://codereview.chromium.org/553050/diff/47/1029
File gpu/pgl/pgl.cc (right):

http://codereview.chromium.org/553050/diff/47/1029#newcode147
gpu/pgl/pgl.cc:147: PGLContext pglCreateContext(NPP npp,
On 2010/01/27 19:45:18, rvargas wrote:
> Out of curiosity, what's the reason for having this code not following our
> coding style?. I mean, everything else looks like our style, but this naming
> style is definitely foreign.

It's using this style:
http://www.khronos.org/registry/egl/api/EGL/egl.h

This is the Pepper equivalent of the standard EGL API. Every version of OpenGL
has some version of this API. Compare also to Apple's AGL and Microsoft's WGL,
which use a similar style.

[darin (slow to review), 2010-01-27 20:57:25]

OK.  By the way, I don't think it is completely unreasonable to compile and
link base/ into a plugin.  We do that for other ordinary test plugins.

-Darin



On Tue, Jan 26, 2010 at 11:56 PM, Alastair Patrick <apatrick@google.com>wrote:

> Yes I am avoiding a dependency on base. This will be built as a NaCl
> untrusted library and linked with NaCl modules so they can use OpenGL on top
> of the NPDevice3D interface. The prevailing idea is that this should not be
> dependent on base. I am beginning to question it.