[GitHub] [cassandra] tatu-at-datastax commented on a change in pull request #1137: [WIP] CASSANDRA-16851: upgrade Jackson 2.9->2.12

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

tatu-at-datastax commented on a change in pull request #1137:
URL: https://github.com/apache/cassandra/pull/1137#discussion_r691626652



##########
File path: src/java/org/apache/cassandra/cql3/Json.java
##########
@@ -42,7 +42,7 @@
      */
     public static String quoteAsJsonString(String s)
     {
-        return new String(BufferRecyclers.getJsonStringEncoder().quoteAsString(s));
+        return new String(JsonStringEncoder.getInstance().quoteAsString(s));

Review comment:
       Good question! I was about to say that there is no performance effect, but going back and looking at code it is not that easy to determine actually. Unfortunately `JsonStringEncoder` change was squarely aimed at usage by Jackson-core itself and not for (unplanned) external use. For Jackson's use there should not be measurable difference given where method is called (only from `SerializedString`, used when constructing serializers).
   
   
   The change did remove use of `ThreadLocal` recycled `TextBuffer` and as a result every encoding does eagerly allocate a `char[120]`; reallocate if needs to expand. This will create bit more garbage than if the underlying working buffer was recycled as in 2.9.
   
   The question then is whether this would have measurable effect, which depends on how often method gets called (like, a hundred times per request or more?) or not.
   
   Given this, I would probably suggest not applying this to 3.x, out of "abundance of caution".
   
   I could otherwise try writing a `jmh` test, but it'd be bit tricky to figure out how to exactly replicate behavior without copying `JsonStringEncoder` implementation (since we'd need different behavior from 2.9 and 2.12).
   And without some sense of actual load it is also difficult to gauge impact.
   
   For trunk it might be enough to rely on full stress/load-testing: I doubt there will be severe slowdown (added memory usage is all transient garbage), but in theory there could be increased GC which could lead to minor observable performance regression.
   
   




-- 
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.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org