[GR-11465] Optimize rb_str_set_len()

PullRequest: truffleruby/534

Author: eregon

spec/ruby/optional/capi/string_spec.rb

@@ -32,44 +32,56 @@ def to_str
     end
   end
 
-  describe "rb_str_set_len" do
-    before :each do
-      # Make a completely new copy of the string
-      # for every example (#dup doesn't cut it).
-      @str = "abcdefghij"[0..-1]
-    end
+  [Encoding::BINARY, Encoding::UTF_8].each do |enc|
+    describe "rb_str_set_len on a #{enc.name} String" do
+      before :each do
+        # Make a completely new copy of the string
+        # for every example (#dup doesn't cut it).
+        @str = "abcdefghij".force_encoding(enc)[0..-1]
+      end
 
-    it "reduces the size of the string" do
-      @s.rb_str_set_len(@str, 5).should == "abcde"
-    end
+      it "reduces the size of the string" do
+        @s.rb_str_set_len(@str, 5).should == "abcde"
+      end
 
-    it "inserts a NULL byte at the length" do
-      @s.rb_str_set_len(@str, 5).should == "abcde"
-      @s.rb_str_set_len(@str, 8).should == "abcde\x00gh"
-    end
+      it "inserts a NULL byte at the length" do
+        @s.rb_str_set_len(@str, 5).should == "abcde"
+        @s.rb_str_set_len(@str, 8).should == "abcde\x00gh"
+      end
 
-    it "updates the byte size and character size" do
-      @s.rb_str_set_len(@str, 4)
-      @str.bytesize.should == 4
-      @str.size.should == 4
-      @str.should == "abcd"
-    end
+      it "updates the byte size" do
+        @s.rb_str_set_len(@str, 4)
+        @str.bytesize.should == 4
+        @str.should == "abcd"
+      end
 
-    it "updates the string's attributes visible in C code" do
-      @s.rb_str_set_len_RSTRING_LEN(@str, 4).should == 4
-    end
+      it "invalidates the character size" do
+        @str.size.should == 10
+        @s.rb_str_set_len(@str, 4)
+        @str.size.should == 4
+        @str.should == "abcd"
+      end
 
-    it "can reveal characters written from C with RSTRING_PTR" do
-      @s.rb_str_set_len(@str, 1)
-      @str.should == "a"
+      it "invalidates the code range" do
+        @s.rb_str_set_len(@str, 4)
+        @str.ascii_only?.should == true
+      end
 
-      @str.force_encoding(Encoding::UTF_8)
-      @s.RSTRING_PTR_set(@str, 1, 'B'.ord)
-      @s.RSTRING_PTR_set(@str, 2, 'C'.ord)
-      @s.rb_str_set_len(@str, 3)
+      it "updates the string's attributes visible in C code" do
+        @s.rb_str_set_len_RSTRING_LEN(@str, 4).should == 4
+      end
+
+      it "can reveal characters written from C with RSTRING_PTR" do
+        @s.rb_str_set_len(@str, 1)
+        @str.should == "a"
 
-      @str.bytesize.should == 3
-      @str.should == "aBC"
+        @s.RSTRING_PTR_set(@str, 1, 'B'.ord)
+        @s.RSTRING_PTR_set(@str, 2, 'C'.ord)
+        @s.rb_str_set_len(@str, 3)
+
+        @str.bytesize.should == 3
+        @str.should == "aBC"
+      end
     end
   end
 

spec/tags/optional/capi/string_tags.txt

@@ -1 +1,2 @@
 fails:SafeStringValue raises for tained string when $SAFE is 1
+fails:C-API String function rb_str_set_len on a ASCII-8BIT String invalidates the code range

src/main/c/cext/ruby.c

@@ -793,7 +793,7 @@ VALUE rb_str_to_str(VALUE string) {
 }
 
 VALUE rb_str_buf_new(long capacity) {
-  VALUE str = rb_str_new(0, capacity);
+  VALUE str = rb_str_new(NULL, capacity);
   rb_str_set_len(str, 0);
   return str;
 }
@@ -812,7 +812,7 @@ VALUE rb_str_concat(VALUE string, VALUE to_concat) {
 
 void rb_str_set_len(VALUE string, long length) {
   long capacity = rb_str_capacity(string);
-  if (length > capacity) {
+  if (length > capacity || length < 0) {
     rb_raise(rb_eRuntimeError, "probable buffer overflow: %ld for %ld", length, capacity);
   }
   rb_tr_wrap(polyglot_invoke(RUBY_CEXT, "rb_str_set_len", rb_tr_unwrap(string), length));

src/main/java/org/truffleruby/cext/CExtNodes.java

@@ -28,6 +28,7 @@
 import com.oracle.truffle.api.profiles.ConditionProfile;
 import com.oracle.truffle.api.source.SourceSection;
 import org.jcodings.Encoding;
+import org.jcodings.specific.ASCIIEncoding;
 import org.jcodings.specific.UTF8Encoding;
 import org.truffleruby.Layouts;
 import org.truffleruby.RubyContext;
@@ -63,6 +64,7 @@
 import org.truffleruby.core.string.StringSupport;
 import org.truffleruby.interop.ToJavaStringNodeGen;
 import org.truffleruby.language.LexicalScope;
+import org.truffleruby.language.NotOptimizedWarningNode;
 import org.truffleruby.language.NotProvided;
 import org.truffleruby.language.RubyBaseNode;
 import org.truffleruby.language.RubyGuards;
@@ -443,15 +445,43 @@ public long capacity(DynamicObject string,
     @CoreMethod(names = "rb_str_set_len", onSingleton = true, required = 2, lowerFixnum = 2)
     public abstract static class RbStrSetLenNode extends CoreMethodArrayArgumentsNode {
 
+        @Child NotOptimizedWarningNode notOptimizedWarningNode;
+
         @Specialization
-        public DynamicObject strSetLen(DynamicObject string, int len,
-                @Cached("create()") StringToNativeNode stringToNativeNode) {
+        public DynamicObject strSetLen(DynamicObject string, int newByteLength,
+                @Cached("create()") StringToNativeNode stringToNativeNode,
+                @Cached("createBinaryProfile()") ConditionProfile binaryProfile) {
             final NativeRope nativeRope = stringToNativeNode.executeToNative(string);
-            final NativeRope newNativeRope = nativeRope.withByteLength(len);
+            final NativeRope newNativeRope;
+
+            if (binaryProfile.profile(nativeRope.getEncoding() == ASCIIEncoding.INSTANCE)) {
+                // TODO (eregon, 17 Jan 2019): We use CR_VALID here as zlib relies on it. Computing
+                // the coderange here on every call is too slow. A proper fix is to compute the
+                // CodeRange lazily on NativeRope.
+                newNativeRope = nativeRope.withByteLength(newByteLength, newByteLength, CodeRange.CR_VALID);
+            } else {
+                performanceWarn("calling rb_str_set_len() on non-binary string is not yet optimized");
+
+                final byte[] bytes = new byte[newByteLength];
+                nativeRope.getNativePointer().readBytes(0, bytes, 0, newByteLength);
+                final long packedLengthAndCodeRange = RopeOperations.calculateCodeRangeAndLength(nativeRope.getEncoding(), bytes, 0, newByteLength);
+                final CodeRange codeRange = CodeRange.fromInt(StringSupport.unpackArg(packedLengthAndCodeRange));
+                final int characterLength = StringSupport.unpackResult(packedLengthAndCodeRange);
+                newNativeRope = nativeRope.withByteLength(newByteLength, characterLength, codeRange);
+            }
+
             StringOperations.setRope(string, newNativeRope);
             return string;
         }
 
+        private void performanceWarn(String message) {
+            if (notOptimizedWarningNode == null) {
+                CompilerDirectives.transferToInterpreterAndInvalidate();
+                notOptimizedWarningNode = insert(new NotOptimizedWarningNode());
+            }
+            notOptimizedWarningNode.warn(message);
+        }
+
     }
 
     @CoreMethod(names = "rb_str_resize", onSingleton = true, required = 2, lowerFixnum = 2)

src/main/java/org/truffleruby/core/rope/NativeRope.java

@@ -11,28 +11,27 @@
 
 import org.jcodings.Encoding;
 import org.truffleruby.core.FinalizationService;
-import org.truffleruby.core.string.StringSupport;
 
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import org.truffleruby.extra.ffi.Pointer;
 
 public class NativeRope extends Rope {
 
-    private Pointer pointer;
+    private final Pointer pointer;
 
     public NativeRope(FinalizationService finalizationService, byte[] bytes, Encoding encoding, int characterLength, CodeRange codeRange) {
-        this(createNativePointer(finalizationService, bytes), bytes.length, encoding, characterLength, codeRange);
+        this(allocateNativePointer(finalizationService, bytes), bytes.length, encoding, characterLength, codeRange);
     }
 
-    private static Pointer createNativePointer(FinalizationService finalizationService, byte[] bytes) {
+    private static Pointer allocateNativePointer(FinalizationService finalizationService, byte[] bytes) {
         final Pointer pointer = Pointer.malloc(bytes.length + 1);
         pointer.enableAutorelease(finalizationService);
         pointer.writeBytes(0, bytes, 0, bytes.length);
         pointer.writeByte(bytes.length, (byte) 0);
         return pointer;
     }
 
-    private static Pointer createNativePointer(FinalizationService finalizationService, Pointer existing) {
+    private static Pointer copyNativePointer(FinalizationService finalizationService, Pointer existing) {
         final Pointer pointer = Pointer.malloc(existing.getSize());
         pointer.enableAutorelease(finalizationService);
         pointer.writeBytes(0, existing, 0, existing.getSize());
@@ -44,22 +43,13 @@ private NativeRope(Pointer pointer, int byteLength, Encoding encoding, int chara
         this.pointer = pointer;
     }
 
-    @TruffleBoundary
-    public NativeRope withByteLength(int newByteLength) {
-        final byte[] bytes = new byte[newByteLength];
-        pointer.readBytes(0, bytes, 0, newByteLength);
-
-        final long packedLengthAndCodeRange = RopeOperations.calculateCodeRangeAndLength(getEncoding(), bytes, 0, newByteLength);
-        final CodeRange codeRange = CodeRange.fromInt(StringSupport.unpackArg(packedLengthAndCodeRange));
-        final int characterLength = StringSupport.unpackResult(packedLengthAndCodeRange);
-
-        getNativePointer().writeByte(newByteLength, (byte) 0); // Like MRI
-
-        return new NativeRope(getNativePointer(), newByteLength, getEncoding(), characterLength, codeRange);
+    public NativeRope withByteLength(int newByteLength, int characterLength, CodeRange codeRange) {
+        pointer.writeByte(newByteLength, (byte) 0); // Like MRI
+        return new NativeRope(pointer, newByteLength, getEncoding(), characterLength, codeRange);
     }
 
     public NativeRope makeCopy(FinalizationService finalizationService) {
-        final Pointer newPointer = createNativePointer(finalizationService, pointer);
+        final Pointer newPointer = copyNativePointer(finalizationService, pointer);
         return new NativeRope(newPointer, byteLength(), getEncoding(), characterLength(), getCodeRange());
     }