[webkit-reviews] review granted: [Bug 182227] Global objects should be able to use TLCs to allocate from different blocks from each other : [Attachment 332588] the patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 182227: Global objects should be able to use TLCs to allocate from
different blocks from each other
https://bugs.webkit.org/show_bug.cgi?id=182227

Attachment 332588: the patch

https://bugs.webkit.org/attachment.cgi?id=332588&action=review




--- Comment #6 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 332588
  --> https://bugs.webkit.org/attachment.cgi?id=332588
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332588&action=review

r=me

> Source/JavaScriptCore/heap/BlockDirectory.cpp:97
> +    }

I don't understand this loop. What changes on each iteration?

> Source/JavaScriptCore/heap/MarkedBlock.h:237
> +	   SecurityOriginToken m_securityOriginToken { 0 };

Is 0 ever a valid token?

> Source/JavaScriptCore/heap/SecurityOriginToken.cpp:36
> +    return WTF::atomicExchangeAdd(&counter, 1) + 1;

Looks like 0 is never valid from this? :-)

> Source/JavaScriptCore/heap/ThreadLocalCache.cpp:35
> +RefPtr<ThreadLocalCache> ThreadLocalCache::create(Heap& heap,
SecurityOriginToken securityOriginToken)

It's weird to pass securityOriginToken in here, since it's a default param in
the header, and IIUC it's never anything else. Could you just not pass it here,
and give it to the ctor call below instead?

> Source/JavaScriptCore/runtime/JSGlobalObject.h:494
> +    JS_EXPORT_PRIVATE explicit JSGlobalObject(VM&, Structure*, const
GlobalObjectMethodTable* = 0);

I love how this used both 0 and nullptr.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:900
> +	       createThreadLocalCache();

This check-and-create isn't racy is it? Seems like it can't if it's a
per-thread thing!