Bug 236124 – cache the result of JSString::toIdentifier

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

JSString::toIdentifier does not store the result of atomizing its string value, except when it is a rope.  We can often end up atomizing the same JSString a number of times.

Forthcoming patch caches the last atomized string produced from JSString::toIdentifier in a given VM.  From local testing, this is a 0.5% Speedometer2 improvement on an M1 MacBook Air, although surprisingly is neutral on a recent Intel MacBook Pro.

[Cameron McCormack (:heycam)]

Created attachment 450862 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/88472619>

[Yusuke Suzuki]

Comment on attachment 450862 [details]
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?

[Cameron McCormack (:heycam)]

(In reply to Yusuke Suzuki from comment #3)
> Is there any reasons why we do not perform it in toAtomString?

No, I think I can move it down to toAtomString.

[Saam Barati]

Comment on attachment 450862 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        JSString::toIdentifier does not store the result of atomizing its string

I wonder if this is worth doing.

[Yusuke Suzuki]

Comment on attachment 450862 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        JSString::toIdentifier does not store the result of atomizing its string
> 
> I wonder if this is worth doing.

This is because string is concurrently accessed from GC thread and concurrent compilers.
If we replace it without stopping them, then these threads will touch the old String => UAF.
If we would like to do that, we need some protocol to allow this.

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #4)
> No, I think I can move it down to toAtomString.

I forgot that's actually what I tried first, but didn't get good results.  I will rename the variables.

[Yusuke Suzuki]

(In reply to Cameron McCormack (:heycam) from comment #7)
> (In reply to Cameron McCormack (:heycam) from comment #4)
> > No, I think I can move it down to toAtomString.
> 
> I forgot that's actually what I tried first, but didn't get good results.  I
> will rename the variables.

Sounds good to me!!

[Cameron McCormack (:heycam)]

Created attachment 451022 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 451023 [details]
Patch

[EWS]

Committed r289177 (246871@main): <https://commits.webkit.org/246871@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451023 [details].