Fast TLS support. (issue6696112)

8 views
Skip to first unread message

vit...@chromium.org

unread,
Mar 25, 2011, 12:33:08 PM3/25/11
to ant...@chromium.org, v8-...@googlegroups.com
Reviewers: antonm,

Description:
Fast TLS support.

This patch adds common infrastructure for fast TLS support and
implementation on win32. More implementations will be added soon.

Fast TLS is controlled by V8_FAST_TLS define which is enabled by
default in our gyp and scons builds. The scons build has
fasttls={on,off} option so that we can see the effects of slow TLS
when needed.

Please review this at http://codereview.chromium.org/6696112/

Affected files:
M SConstruct
M src/isolate.h
A src/platform-tls-win32.h
A src/platform-tls.h
M src/platform.h
M tools/gyp/v8.gyp
M tools/v8.xcodeproj/project.pbxproj
M tools/visual_studio/v8_base.vcproj


ant...@chromium.org

unread,
Mar 25, 2011, 12:44:44 PM3/25/11
to vit...@chromium.org, v8-...@googlegroups.com
LGTM

I hope that this fast way to read TLS is covered by the current tests as
it's
quite a common path, correct?


http://codereview.chromium.org/6696112/diff/1/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6696112/diff/1/SConstruct#newcode794
SConstruct:794: 'help': 'enable fast thread local storage support'
maybe add something like 'if supported for the given platform'

http://codereview.chromium.org/6696112/diff/1/src/platform-tls-win32.h
File src/platform-tls-win32.h (right):

http://codereview.chromium.org/6696112/diff/1/src/platform-tls-win32.h#newcode43
src/platform-tls-win32.h:43: const intptr_t kTibInlineTlsOffset = 0xE10;
nit: do not we use lower case in hex constants?

http://codereview.chromium.org/6696112/diff/1/src/platform-tls.h
File src/platform-tls.h (right):

http://codereview.chromium.org/6696112/diff/1/src/platform-tls.h#newcode39
src/platform-tls.h:39: // provided fast TLS support for the current
platform and architecture
nit: provided -> provides?

http://codereview.chromium.org/6696112/diff/1/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/6696112/diff/1/src/platform.h#newcode433
src/platform.h:433: static inline void*
GetExistingThreadLocal(LocalStorageKey key) {
maybe make this method a template by a return type?

http://codereview.chromium.org/6696112/

vit...@chromium.org

unread,
Mar 27, 2011, 11:54:48 AM3/27/11
to ant...@chromium.org, v8-...@googlegroups.com
Anton, thanks for reviewing this! I addressed most of your comments and
added a
unit test for TLS stores/reads.


-- Vitaly

http://codereview.chromium.org/6696112/diff/1/SConstruct#newcode794
SConstruct:794: 'help': 'enable fast thread local storage support'

On 2011/03/25 16:44:44, antonm wrote:
> maybe add something like 'if supported for the given platform'

Done.

http://codereview.chromium.org/6696112/diff/1/src/platform-tls-win32.h#newcode43
src/platform-tls-win32.h:43: const intptr_t kTibInlineTlsOffset = 0xE10;

On 2011/03/25 16:44:44, antonm wrote:
> nit: do not we use lower case in hex constants?

Upper case seems to be more common.

http://codereview.chromium.org/6696112/diff/1/src/platform-tls.h#newcode39
src/platform-tls.h:39: // provided fast TLS support for the current
platform and architecture

On 2011/03/25 16:44:44, antonm wrote:
> nit: provided -> provides?

Done.

http://codereview.chromium.org/6696112/diff/1/src/platform.h#newcode433
src/platform.h:433: static inline void*
GetExistingThreadLocal(LocalStorageKey key) {

On 2011/03/25 16:44:44, antonm wrote:
> maybe make this method a template by a return type?

I'd like to avoid significant interface changes in this patch.

http://codereview.chromium.org/6696112/

Reply all
Reply to author
Forward
0 new messages