CASSANDRA-16851: upgrade Jackson 2.9->2.12 #1137
Conversation
cowtowncoder
changed the title
|
Whops. Used wrong issue id in commit, fixed in title. |
cowtowncoder
changed the title
|
As per @ekaterinadimitrova2 's suggestion, try targeting the latest stable Jackson release, 2.12(.4). |
| @@ -42,7 +42,7 @@ | |||
| */ | |||
| public static String quoteAsJsonString(String s) | |||
| { | |||
| return new String(BufferRecyclers.getJsonStringEncoder().quoteAsString(s)); | |||
| return new String(JsonStringEncoder.getInstance().quoteAsString(s)); | |||
There was a problem hiding this comment.
Is there a performance impact from these change? (I don't know the difference between BufferRecyclers and JsonStringEncoder)
My concern is that this is used at a per-column and per-request level, and a performance change could result in bringing a cluster down… (especially if we apply this to release versions)
There was a problem hiding this 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.
There was a problem hiding this comment.
Oh. Forgot to add the important piece: change is necessary since 2.10 does not have the old method at all.
(this was an accidental compatibility breakage, actually; method should have been marked as Deprecated, and only removed in 3.0... but was not reported by anyone prior to release of 2.10 AFAIK).
So leaving use of the old method is unfortunately not an option.
There was a problem hiding this comment.
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.
Then probably better to do it for trunk only probably? I expect that there the big in the game probably will test significantly before a major release to catch any regressions? @michaelsembwever , WDYT about that? I am uncertain to be honest
There was a problem hiding this comment.
One other thing that might be relevant: depending on timing, we might get Jackson 2.12.5 patch, which has one improvement wrt this change:
as I plan to publish 2.12.5 relatively soon, due to another unrelated fix.
Possibly it would get released as soon as upcoming weekend.
I am also curious whether trunk is likely to become 4.1, or 5.0 -- I'd guess 4.1 at this point?
There was a problem hiding this comment.
Thanks for the update, I also find it worth it to wait that new version if it's planned soon.
I think the idea about trunk is to be 4.1 until someone commits something for 5.0, then the plan changes. @michaelsembwever knows the best
There was a problem hiding this comment.
I am also curious whether trunk is likely to become 4.1, or 5.0 -- I'd guess 4.1 at this point?
It can be either, but for now it is named 4.1 (and it will be renamed to 5.0 if warranted).
There was a problem hiding this comment.
Can we write a simple JMH microbench class to test the performance difference? That would be enough for me.
Like just a copy of https://github.com/apache/cassandra/blob/trunk/test/microbench/org/apache/cassandra/test/microbench/AutoBoxingBench.java
(The microbench full run almost works, ref: https://ci-cassandra.apache.org/view/Cassandra%204.1/job/Cassandra-trunk-microbench/
But they are easy enough to run individually locally, which is needed anymore for usable results.)
There was a problem hiding this comment.
The main challenge wrt jmh test (which is a good idea otherwise) is that the implementation between Jackson 2.9 and 2.10 and later differ and I cannot think of a way to test difference with just, say, Jackson 2.9. Buffer reuse is based on ThreadLocal for 2.9; this is removed from 2.10 but some other things are changed as well (default size of buffer allocated, I think).
There was a problem hiding this comment.
With 2.12.5 I can revert this change as I added back the method in question for backwards-compatibility.
Ideally (in future) call will be changed, but it is lower risk doing minimal change.
cowtowncoder
changed the title
|
@michaelsembwever As per above comment, decided to go with minimal diff here. A full CI run would make sense now, I think? |
|
Merged outside of this PR (into 3.11, 4.0, trunk), closing PR. |
As per title, patch to upgrade Jackson version from the latest 2.9 patch to the latest 2.10 patch.
One change required wrt access of
JsonStringEncoder.Assuming that
trunkis for Cassandra 4.1: while this should be safe change seems better than for 4.0.x.I am running the test suite locally which seems to take a while. Not sure if there's Cassandra CI of some kind for other tests; will update if and when test suite passes (or if there are failures). Leaving as [WIP] until then.