Avoid String.format usages in hot paths #15258
Conversation
pettyjamesm
force-pushed
the
reduce-string-format-usages
branch
2 times, most recently
from
95aa4e3 to
6644105
Compare
core/trino-spi/src/main/java/io/trino/spi/type/TimeZoneKey.java
Outdated
Show resolved
Hide resolved
| @Fork(1) | ||
| @Warmup(iterations = 5, time = 1, timeUnit = SECONDS) | ||
| @Measurement(iterations = 5, time = 1, timeUnit = SECONDS) | ||
| public class BenchmarkCastTimestampToVarchar |
There was a problem hiding this comment.
Why not benchmark the cast methods directly instead of going through the expression compiler and processor machinery?
There was a problem hiding this comment.
I thought about it, but initially I did it this way to make sure I was actually changing the correct functions that would be used during casting operations. After I was sure that I was, it turned out actually to be easier to do it this way since I could test all of the different types with a single benchmark instead of having to specialize benchmark methods for each type as well as short/long variants of timestamps which end up dispatching to different methods based on precision.
| for (int index = builder.length() - 1; index > 8; index--) { | ||
| long temp = scaledFraction / 10; | ||
| int digit = (int) (scaledFraction - (temp * 10)); | ||
| scaledFraction = temp; | ||
| builder.setCharAt(index, (char) ('0' + digit)); |
There was a problem hiding this comment.
Didn't we add similar code somewhere else recently? We may want to consider putting it somewhere we can reuse it, since it's not particularly trivial.
There was a problem hiding this comment.
We did indeed. That change was to SqlTimestamp in the SPI which improved performance just for the JSON serialization path. I think there are a few problems that I think has caused this code to proliferate:
- SqlTimestamp and SqlTimestampWithTimeZone are SPI objects that seem to only exist in order to serialize values as JSON. They have similar, but slightly different logic from one another (the time zone part)
- Casting to timestamp (and with time zone) types are currently implemented in trino-main and also formatted to strings (even though casting to varchar needs a UTF8 slice) because the formatting logic is being shared for long and short timestamp types both with and without time zones and for conversions and in parts of the JSON type handling paths. So it’s a 2x2 grid of timestamp types each targeting at least 2 output representations, and not accessible from the SPI - meanwhile the SPI can’t host the implementation because it depends on the packing / unpacking implementations in main.
- Time with and without time zone is similar in Trino main, except that was already producing a slice directly (presumably for performance to avoid the intermediate string and since the format is simple enough to implement by hand) which means it also can’t live in the SPI because of the dependency on slice.
pettyjamesm
force-pushed
the
reduce-string-format-usages
branch
from
6644105 to
773fbea
Compare
Description
Replaces usages of
String.formatin expected codepaths to improve performance. In particulartime/timestamprelated JSON serializers and casts tovarcharimprove dramatically withtimestamp(3)casting throughput improving 2x,time(3)improving more than 30x, andtime with time zone(3)improving ~40x (benchmark results: jmh.morethan.io).Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: