[webkit-reviews] review granted: [Bug 205544] Replace uses of Box<Identifier> with a new CacheableIdentifier class. : [Attachment 387410] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 205544: Replace uses of Box<Identifier> with a new CacheableIdentifier
class.
https://bugs.webkit.org/show_bug.cgi?id=205544

Attachment 387410: proposed patch.

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




--- Comment #20 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 387410
  --> https://bugs.webkit.org/attachment.cgi?id=387410
proposed patch.

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

r=me

Looks really good overall. Just two main comments where things are iffy

> Source/JavaScriptCore/ChangeLog:9
> +	   The introduction of the use of Box<Identifier> is to get around
having to

"is to get" => "was to get"

> Source/JavaScriptCore/ChangeLog:16
> +	   This patch fixes this by replacing uses of Box<Identifier> with a

"with a" => "with"

> Source/JavaScriptCore/ChangeLog:79
> +	   The GCSafeConcurrentJSLocker ensures that the CacheableIdentifier is
will be

"is will be" => will be"

That's just a lock. How does it ensure it'll be protected from GC? (I know the
answer, that stronglyVisitStrongReferences holds the lock, but this sentence is
slightly confusing because it doesn't indicate that.)

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:2
> + * Copyright (C) 2017-2019 Apple Inc. All rights reserved.

I think these should be 2020 (you have a few 2019)

> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:71
> +    m_getByIdSelfIdentifier = identifier;
> +    codeBlock->vm().heap.writeBarrier(codeBlock);

I wonder if somehow the compiler could enforce this. I guess it's difficult
because we use them all over the compiler thread.

> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:239
> +    if (ident.isCell())
> +	   keepAlive(ident.cell());

I'm not sure what I'm missing, but where did this keepAlive come from?

> Source/JavaScriptCore/jit/JITOperations.cpp:2032
> +    if (baseValue.isCell() &&
CacheableIdentifier::isCacheableIdentifierCell(subscript)) {

If we see a string that's not a cacheable identifier, should we bail on the IC
(e.g, make it just go. to the slow path)?

> Source/JavaScriptCore/runtime/CacheableIdentifier.h:89
> +    static constexpr uintptr_t s_uidTag = 1;

nit: Might be worth a comment saying why we tag the UID instead of the cell,
since it will be part of conservative GC this way, which is a nice property

> Source/JavaScriptCore/runtime/VM.h:1208
> +    HashMap<const UniquedStringImpl*, RefPtr<WatchpointSet>>
m_impurePropertyWatchpointSets;

this looks wrong. You're no longer +1 reffing the string