[Truffle] Removed the string_copy_from primitive.

This primitive was really designed for a byte-oriented String API and our implementation for ropes was not very good. Moreover, it can be modeled straightforwardly as a form of splicing and splicing has been updated to work with ropes.
jruby · Dec 21, 2016 · 12c6ebf · 12c6ebf
1 parent 8180861
commit 12c6ebf
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 60 deletions.
diff --git a/truffle/src/main/java/org/jruby/truffle/core/string/StringNodes.java b/truffle/src/main/java/org/jruby/truffle/core/string/StringNodes.java
@@ -1185,7 +1185,6 @@ public ForceEncodingNode(RubyContext context, SourceSection sourceSection) {
             withEncodingNode = RopeNodesFactory.WithEncodingNodeGen.create(null, null, null);
         }
 
-        @TruffleBoundary
         @Specialization(guards = "isRubyString(encodingName)")
         public DynamicObject forceEncodingString(DynamicObject string, DynamicObject encodingName,
                                                  @Cached("createBinaryProfile()") ConditionProfile differentEncodingProfile,
@@ -3829,63 +3828,6 @@ public Object other(DynamicObject string, int index) {
 
     }
 
-    @Primitive(name = "string_copy_from", needsSelf = false, lowerFixnum = { 3, 4, 5 })
-    public static abstract class StringCopyFromPrimitiveNode extends PrimitiveArrayArgumentsNode {
-
-        @Specialization(guards = { "isRubyString(other)", "size >= 0", "!offsetTooLarge(start, other)", "!offsetTooLargeRaw(dest, string)" })
-        public DynamicObject stringCopyFrom(DynamicObject string, DynamicObject other, int start, int size, int dest,
-                                            @Cached("createBinaryProfile()") ConditionProfile negativeStartOffsetProfile,
-                                            @Cached("createBinaryProfile()") ConditionProfile sizeTooLargeInReplacementProfile,
-                                            @Cached("createBinaryProfile()") ConditionProfile negativeDestinationOffsetProfile,
-                                            @Cached("createBinaryProfile()") ConditionProfile sizeTooLargeInStringProfile) {
-            // Taken from Rubinius's String::copy_from.
-
-            int src = start;
-            int dst = dest;
-            int cnt = size;
-
-            final Rope otherRope = rope(other);
-            int osz = otherRope.byteLength();
-            if(negativeStartOffsetProfile.profile(src < 0)) src = 0;
-            if(sizeTooLargeInReplacementProfile.profile(cnt > osz - src)) cnt = osz - src;
-
-            final ByteList stringBytes = RopeOperations.toByteListCopy(Layouts.STRING.getRope(string));
-            int sz = stringBytes.unsafeBytes().length - stringBytes.begin();
-            if(negativeDestinationOffsetProfile.profile(dst < 0)) dst = 0;
-            if(sizeTooLargeInStringProfile.profile(cnt > sz - dst)) cnt = sz - dst;
-
-            System.arraycopy(otherRope.getBytes(), src, stringBytes.getUnsafeBytes(), stringBytes.begin() + dest, cnt);
-
-            StringOperations.setRope(string, StringOperations.ropeFromByteList(stringBytes));
-
-            return string;
-        }
-
-        @Specialization(guards = { "isRubyString(other)", "size < 0 || (offsetTooLarge(start, other) || offsetTooLargeRaw(dest, string))" })
-        public DynamicObject stringCopyFromWithNegativeSize(DynamicObject string, DynamicObject other, int start, int size, int dest) {
-            return string;
-        }
-
-        protected boolean offsetTooLarge(int offset, DynamicObject string) {
-            assert RubyGuards.isRubyString(string);
-
-            return offset >= Layouts.STRING.getRope(string).byteLength();
-        }
-
-        protected boolean offsetTooLargeRaw(int offset, DynamicObject string) {
-            assert RubyGuards.isRubyString(string);
-
-            // This bounds checks on the total capacity rather than the virtual
-            // size() of the String. This allows for string adjustment within
-            // the capacity without having to change the virtual size first.
-
-            // TODO (nirvdrum 21-Jan-16) Verify whether we still need this method as we never have spare capacity allocated with ropes.
-            final Rope rope = rope(string);
-            return offset >= rope.byteLength();
-        }
-
-    }
-
     @Primitive(name = "string_rindex", lowerFixnum = 2)
     public static abstract class StringRindexPrimitiveNode extends PrimitiveArrayArgumentsNode {
 

diff --git a/truffle/src/main/ruby/core/string_mirror.rb b/truffle/src/main/ruby/core/string_mirror.rb
@@ -57,8 +57,16 @@ def previous_byte_index(index)
         Truffle.invoke_primitive :string_previous_byte_index, @object, index
       end
 
-      def copy_from(other, start, size, dest)
-        Truffle.invoke_primitive :string_copy_from, @object, other, start, size, dest
+      def copy_from(other, other_offset, byte_count_to_copy, dest_offset)
+        sz = @object.bytesize
+        osz = other.bytesize
+
+        other_offset = 0 if other_offset < 0
+        dest_offset = 0 if dest_offset < 0
+        byte_count_to_copy = osz - other_offset if byte_count_to_copy > osz - other_offset
+        byte_count_to_copy = sz - dest_offset if byte_count_to_copy > sz - dest_offset
+
+        splice(dest_offset, byte_count_to_copy, other.byteslice(other_offset, byte_count_to_copy))
       end
 
       def splice(starting_byte_index, byte_count_to_replace, replacement, encoding=nil)