Improve Ruby string interning #3185

nirvdrum · 2023-07-26T14:41:03Z

This PR improves the Ruby string interning process by allowing look-ups using substrings without having to extract their bytes. It also switches to using TruffleString's InternalByteArray, allowing string interning to work on native strings (I haven't run into a case requiring this yet).

nirvdrum · 2023-07-26T14:42:12Z

src/main/java/org/truffleruby/core/string/TBytesKey.java

@@ -62,4 +91,51 @@ public String toString() {
        return TruffleString.fromByteArrayUncached(bytes, encoding, false).toString();
    }

+    private static int hashCode(InternalByteArray byteArray) {


I think it would be nice if we could upstream this into Truffle's ArrayUtils.

I think in InternalByteArray would be a better place, TruffleString has the byte[] intrinsics nowadays.

That'd be fine, too. There had been some concerns previously about exposing public APIs in something named "internal", but I don't really care where it goes.

nirvdrum · 2023-07-26T14:42:59Z

src/main/java/org/truffleruby/core/string/TBytesKey.java


 public final class TBytesKey {

    private final byte[] bytes;
+    private final int offset;


Since we cannot construct TruffleString's InternalByteArray, we basically have to recreate it here. It'd be nice if there were a cleaner option.

On the plus side this actually consumes less memory (i.e., we don't keep the InternalByteArray instance around).

eregon

Right it makes sense to be able to lookup without copying bytes.
OTOH when adding an entry in the cache it should copy if not a perfect fit, to avoid holding onto extra hidden bytes "outside the substring". The PR already handles that, nice.

eregon · 2023-07-28T15:44:09Z

src/main/java/org/truffleruby/core/string/TBytesKey.java


 public final class TBytesKey {

    private final byte[] bytes;
+    private final int offset;


On the plus side this actually consumes less memory (i.e., we don't keep the InternalByteArray instance around).

eregon · 2023-07-28T15:46:24Z

src/main/java/org/truffleruby/core/string/TBytesKey.java

+        if (a.isPerfectFit() && b.isPerfectFit()) {
+            return Arrays.equals(a.bytes, b.bytes);
+        }


There doesn't seem to be a big perf advantage here so I'd just remove this and use the general case below.

Will Graal optimize both cases? Array.equals(byte[], byte[]) is annotated as @IntrinsicCandidate, while the variant with specified offsets and end points is not. I thought that would be useful for the interpreter, at least. Granted, I didn't benchmark the two.

They both end up in vectorizedMismatch and that's intrinsified by Graal, yes

Arrays.equals(byte[], byte[]) is also intrinsified by Graal so maybe it's a tiny bit better.
I guess the only way to know for sure is to benchmark it.

It's fine as it is, no need to spend too much time on it, either way is fine.

eregon · 2023-07-28T15:51:36Z

src/main/java/org/truffleruby/core/string/TStringCache.java

    }

-    @TruffleBoundary
    public TruffleString getTString(byte[] bytes, RubyEncoding rubyEncoding) {


Should still be @TruffleBoundary, it's counter-productive to allocate the TBytesKey in PE code.

I really wish we had a reason field on @TruffleBoundary because it's easy to look at this and say "there's no reason this can't be compiled, so lets do away with the boundary". I've been operating under the assumption that anything that could run without a boundary should and let Truffle's heuristics sort out the rest.

Feel free to add a code comment about it. Here it's a case of "no value to PE this code, and worse due to forcing an extra allocation that might not be needed otherwise". We should only PE what can benefit from PE, there is a warmup cost to PE too much code.

src/main/java/org/truffleruby/core/string/TStringCache.java

eregon · 2023-07-28T15:59:41Z

src/main/java/org/truffleruby/core/string/TBytesKey.java

@@ -62,4 +91,51 @@ public String toString() {
        return TruffleString.fromByteArrayUncached(bytes, encoding, false).toString();
    }

+    private static int hashCode(InternalByteArray byteArray) {


I think in InternalByteArray would be a better place, TruffleString has the byte[] intrinsics nowadays.

src/main/java/org/truffleruby/core/string/TStringCache.java

eregon · 2023-07-28T16:01:45Z

src/main/java/org/truffleruby/core/string/TBytesKey.java


 public final class TBytesKey {

    private final byte[] bytes;
+    private final int offset;
+    private final int length;
+    private final boolean isImmutable;


I think this should be tracked externally and passed to makeCacheable(), to save footprint.

eregon · 2023-07-28T16:09:07Z

I'm curious, did you notice the current approach was a bottleneck in some benchmark?

nirvdrum · 2023-08-03T05:53:04Z

Improving String#-@ came up while continuing working on #2089. YAML loading of i18n data adds a lot of time to our test setup. I used the script from #2089 (comment) and profiled with that.

The savings wasn't as pronounced as I had hoped for. Looking at the SVG from jt profile (collected before addressing PR feedback) we have:

Case	Self Samples %
Before	4.53%
After	3.95%

In MRI, the string interning process accounts for a trivial amount of total execution. We have more going on with the concurrent collection, but I wanted to try to reduce our overhead as much as possible. Reducing memory copies looked like a straightforward improvement.

Setting aside the i18n YAML loading, I noticed that RubyGems will intern a few small substrings repeatedly. The impact of that is much harder to measure. I'm assuming doing less work will yield better results in this case.

Previously, we always extracted the precise range of bytes we needed when looking up an entry in the frozen string table. If an entry already existed, we'd discard that extracted range in favor of what was already in the cache. This change defers making a copy of the string's bytes until we need to insert into the cache. For situations with many cache hits this approach can be much faster.

…ity.

…ble. By restructuring to work with `InternalByteArray`, we can now support working with native strings as well. The `InternalByteArray` will make a copy of the native memory into a Java `byte[]`, which won't change behind our backs.

… cache key to save memory.

eregon · 2023-08-10T09:27:11Z

src/main/java/org/truffleruby/core/string/TBytesKey.java

 import org.truffleruby.core.encoding.RubyEncoding;
+import org.truffleruby.core.encoding.TStringUtils;

 public final class TBytesKey {


Could you document here or somewhere else why we support offset and length? (to avoid extra copying for substrings during lookup, but not when inserting a new entry, ref #makeCacheable)

eregon · 2023-08-10T09:28:13Z

src/main/java/org/truffleruby/core/string/TBytesKey.java

+        return offset == 0 && length == bytes.length;
+    }
+
+    public TBytesKey makeCacheable(boolean isImmutable) {


Could you document this? (it creates a perfect fit key, so it does not hold on any extra bytes that it does not need when inserting a new entry to avoid leaking bytes outside the substring)

nirvdrum · 2023-08-17T23:10:14Z

Feedback address in #3216. I messed up the PR management and GitHub won't let me re-open this PR.

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jul 26, 2023

nirvdrum commented Jul 26, 2023

View reviewed changes

nirvdrum requested a review from eregon July 26, 2023 14:43

nirvdrum added shopify performance labels Jul 26, 2023

nirvdrum force-pushed the improve-ruby-string-intern branch from 94de08d to b8efd14 Compare July 26, 2023 15:45

eregon reviewed Jul 28, 2023

View reviewed changes

eregon self-assigned this Jul 28, 2023

nirvdrum added 5 commits August 7, 2023 23:31

Use helper method for checking if TruffleString is immutable for clar…

ce9d91b

…ity.

Move the immutability tracking for interned string lookups out of the…

c19108e

… cache key to save memory.

Fix a typo.

35817af

nirvdrum force-pushed the improve-ruby-string-intern branch from b8efd14 to 35817af Compare August 8, 2023 03:35

eregon reviewed Aug 10, 2023

View reviewed changes

nirvdrum closed this Aug 11, 2023

nirvdrum deleted the improve-ruby-string-intern branch August 11, 2023 06:38

nirvdrum mentioned this pull request Aug 16, 2023

Improve Ruby string interning #3216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Ruby string interning #3185

Improve Ruby string interning #3185

nirvdrum commented Jul 26, 2023

nirvdrum Jul 26, 2023

eregon Jul 28, 2023

nirvdrum Aug 3, 2023

nirvdrum Jul 26, 2023

eregon Jul 28, 2023

eregon left a comment

eregon Jul 28, 2023

eregon Jul 28, 2023

nirvdrum Aug 8, 2023 •

edited

Loading

eregon Aug 10, 2023

eregon Aug 10, 2023

eregon Aug 14, 2023

eregon Jul 28, 2023

nirvdrum Aug 8, 2023

eregon Aug 10, 2023

eregon Jul 28, 2023

eregon Jul 28, 2023

eregon commented Jul 28, 2023

nirvdrum commented Aug 3, 2023

eregon Aug 10, 2023

eregon Aug 10, 2023

nirvdrum commented Aug 17, 2023

Improve Ruby string interning #3185

Improve Ruby string interning #3185

Conversation

nirvdrum commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirvdrum Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Jul 28, 2023

nirvdrum commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirvdrum commented Aug 17, 2023

nirvdrum Aug 8, 2023 •

edited

Loading