[GitHub] [flink] pnowojski commented on a change in pull request #10529: [FLINK-15171] [serialization] fix performance regression caused by too many buffer allocations on string serialization

[GitBox <gi...@apache.org>]

pnowojski commented on a change in pull request #10529: [FLINK-15171] [serialization] fix performance regression caused by too many buffer allocations on string serialization
URL: https://github.com/apache/flink/pull/10529#discussion_r357918705
 
 

 ##########
 File path: flink-core/src/main/java/org/apache/flink/types/StringValue.java
 ##########
 @@ -776,9 +784,16 @@ public static String readString(DataInput in) throws IOException {
 		 * to accommodate for the next characters.
 		 */
 
-		// happily assume that the string is an 7 bit us-ascii one
-		byte[] buf = new byte[len];
-		in.readFully(buf);
+		byte[] buf;
+		if (len < SHORT_STRING_MAX_LENGTH) {
+			// skip allocating a separate buffer and reuse the thread-local one.
+			// as allocating the buffer for small strings can produce too much GC pressure.
+			buf = shortStringBuffer.get();
+			in.readFully(buf, 0, len);
+		} else {
+			buf = new byte[len];
+			in.readFully(buf);
+		}
 
 Review comment:
   Hey. Definitely one issue with this approach is that we do not have benchmark coverage for strings > 1KB. `org.apache.flink.benchmark.SerializationFrameworkMiniBenchmarks#serializerHeavyString` as I can see is using just 1KB strings? Maybe it just barely qualifies for the non `ThreadLocal` branch, but if we wanted to go such direction, we would need to either decrease the size of the buffer (100?) or incease strings in `serializerHeavyString` (`SHORT_STRING_MAX_LENGTH * 1.5` ?).
   
   However do we need such differentiation? Couldn't this have this one single per thread fixed size buffer, but if the string is larger the buffer max size, read/write string from/to the fixed size buffer size in steps? like:
   ```
   for step in 0..string.size()/buf.length:
     for i in 0..buf.length:
        buf[i] = string[step * buf.length + i] // or in.readFully?
     // process buffer
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services