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.
Created attachment 450862 [details] patch
<rdar://problem/88472619>
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?
(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.
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.
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.
(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.
(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!!
Created attachment 451022 [details] Patch
Created attachment 451023 [details] Patch
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].