Speed up time interval arounding around dst (backport #56371) #56396
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When an index spans a daylight savings time transition we can't use our optimization that rewrites the requested time zone to a fixed time zone and instead we used to fall back to a java.util.time based rounding implementation. In elastic#55559 we optimized "time unit" rounding. This optimizes "time interval" rounding. The java.util.time based implementation is about 1650% slower than the rounding implementation for a fixed time zone. This replaces it with a similar optimization that is only about 30% slower than the fixed time zone. The java.util.time implementation allocates a ton of short lived objects but the optimized implementation doesn't. So it *might* end up being faster than the microbenchmarks imply.
nik9000
changed the title
This fixes a bug in the interval rounding and two test bugs that showed up when I ran 1000s of iterations of the tests. The bug was that we could end up with duplicate transitions if we only needed the last transition from the list of fully defined transitions and the transitions overlapped with the rules. This happens. Its ok. We had defence against that if we needed more than one transition but forgot it in this case. Now we have it and assertions to make sure we catch any similar mistakes. One test bug had to do with the randomized test calling `round(round(min))` because sometimes the first `round` call will return a time less than min. Specifically if `min` is near daylight savings time. Anyway, this change fixes it by building an extra-wide prepared rounding just in case. The other test bug came up when adding a test for the real bug, namely that `assertRoundingAtOffset` made an assertion that wasn't true if the time to round ended up being in an overlap. This just drops that assertion. We have similar assertions in cases where we *know* what kind of offsets we're working with in other tests. Closes elastic#56400
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an index spans a daylight savings time transition we can't use our
optimization that rewrites the requested time zone to a fixed time zone
and instead we used to fall back to a java.util.time based rounding
implementation. In #55559 we optimized "time unit" rounding. This
optimizes "time interval" rounding.
The java.util.time based implementation is about 1650% slower than the
rounding implementation for a fixed time zone. This replaces it with a
similar optimization that is only about 30% slower than the fixed time
zone. The java.util.time implementation allocates a ton of short lived
objects but the optimized implementation doesn't. So it might end up
being faster than the microbenchmarks imply.