[webkit-reviews] review granted: [Bug 236124] cache the result of JSString::toIdentifier : [Attachment 450862] patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Cameron McCormack (:heycam)
<heycam at apple.com>'s request for review:
Bug 236124: cache the result of JSString::toIdentifier
https://bugs.webkit.org/show_bug.cgi?id=236124

Attachment 450862: patch

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




--- Comment #3 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 450862
  --> https://bugs.webkit.org/attachment.cgi?id=450862
patch

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

Nice, I have some questions.

> Source/JavaScriptCore/runtime/JSString.h:785
> +    if (vm.lastAtomizedString.ptr() != valueInternal().impl()) {
> +	   vm.lastAtomizedString = *valueInternal().impl();
> +	   vm.lastAtomizedStringAtom =
AtomStringImpl::add(valueInternal().impl()).releaseNonNull();
> +    }

Is there any reasons why we do not perform it in toAtomString?
If we should do this cache only in `toIdentifier` call and we should do that
only when converting existing non-atom-string to atom-string, then can we
change	lastAtomizedString / lastAtomizedStringAtom names to more specific
ones?