Skip to content

Commit 413a7fe

Browse files
committed
Performance: improve event.clone memory usage
for Strings with copy-on-write semantics when deep cloning. motivated by "big" events reaching plugins such as split, which might produce several new events out of a single one. Fixes #11794
1 parent fb8108e commit 413a7fe

File tree

2 files changed

+57
-6
lines changed

2 files changed

+57
-6
lines changed

logstash-core/src/main/java/org/logstash/Cloner.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ public static <T> T deep(final T input) {
4242
} else if (input instanceof List<?>) {
4343
return (T) deepList((List<?>) input);
4444
} else if (input instanceof RubyString) {
45-
return (T) ((RubyString) input).doClone();
45+
// new instance but sharing ByteList (until either String is modified)
46+
return (T) ((RubyString) input).dup();
4647
} else if (input instanceof Collection<?>) {
4748
throw new ClassCastException("unexpected Collection type " + input.getClass());
4849
}

logstash-core/src/test/java/org/logstash/ClonerTest.java

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121
package org.logstash;
2222

2323
import org.jruby.RubyString;
24+
import org.jruby.runtime.ThreadContext;
25+
import org.jruby.runtime.builtin.IRubyObject;
26+
import org.jruby.util.ByteList;
2427
import org.junit.Test;
2528

2629
import static org.junit.Assert.*;
@@ -33,14 +36,61 @@ public void testRubyStringCloning() {
3336

3437
RubyString result = Cloner.deep(original);
3538
// Check object identity
36-
assertTrue(result != original);
39+
assertNotSame(original, result);
40+
// Check string equality
41+
assertEquals(original, result);
3742

38-
// Check different underlying bytes
39-
assertTrue(result.getByteList() != original.getByteList());
43+
assertEquals(javaString, result.asJavaString());
44+
}
4045

41-
// Check string equality
42-
assertEquals(result, original);
46+
@Test
47+
public void testRubyStringCloningAndAppend() {
48+
String javaString = "fooBar";
49+
RubyString original = RubyString.newString(RubyUtil.RUBY, javaString);
4350

51+
RubyString result = Cloner.deep(original);
52+
53+
result.append(RubyUtil.RUBY.newString("X"));
54+
55+
assertNotEquals(result, original);
56+
57+
ThreadContext context = RubyUtil.RUBY.getCurrentContext();
58+
assertTrue(original.op_equal(context, RubyString.newString(RubyUtil.RUBY, javaString)).isTrue());
59+
assertEquals(javaString, original.asJavaString());
60+
}
61+
62+
@Test
63+
public void testRubyStringCloningAndChangeOriginal() {
64+
String javaString = "fooBar";
65+
RubyString original = RubyString.newString(RubyUtil.RUBY, javaString);
66+
67+
RubyString result = Cloner.deep(original);
68+
69+
ThreadContext context = RubyUtil.RUBY.getCurrentContext();
70+
IRubyObject index = RubyUtil.RUBY.newFixnum(5);
71+
original.op_aset(context, index, RubyUtil.RUBY.newString("z")); // original[5] = 'z'
72+
73+
assertNotEquals(result, original);
74+
75+
assertTrue(result.op_equal(context, RubyString.newString(RubyUtil.RUBY, javaString)).isTrue());
4476
assertEquals(javaString, result.asJavaString());
77+
assertEquals("fooBaz", original.asJavaString());
78+
}
79+
80+
@Test // @Tag("Performance Optimization")
81+
public void testRubyStringCloningMemoryOptimization() {
82+
ByteList bytes = ByteList.create("0123456789");
83+
RubyString original = RubyString.newString(RubyUtil.RUBY, bytes);
84+
85+
RubyString result = Cloner.deep(original);
86+
assertNotSame(original, result);
87+
88+
assertSame(bytes, original.getByteList());
89+
// NOTE: this is an implementation detail or the underlying sharing :
90+
assertSame(bytes, result.getByteList()); // bytes-list shared
91+
92+
// but when string is modified it will stop using the same byte container
93+
result.concat(RubyUtil.RUBY.getCurrentContext(), RubyUtil.RUBY.newString(" "));
94+
assertNotSame(bytes, result.getByteList()); // byte-list copied on write
4595
}
4696
}

0 commit comments

Comments
 (0)