Optimize date rounding #128687
Optimize date rounding #128687
Conversation
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| long roundFloor(long utcMillis, int multiplier) { | ||
| return multiplier == 1 ? DateUtils.roundYear(utcMillis) : DateUtils.roundYearInterval(utcMillis, multiplier); | ||
| assert multiplier == 1; | ||
| return DateUtils.roundYear(utcMillis); |
There was a problem hiding this comment.
Please confirm my understanding that this check was showing in profile?
There was a problem hiding this comment.
Not that one specifically, but the equivalent for WEEK_OF_YEAR, in particular I see the cost of roundWeekIntervalOfWeekYear(). It's smaller than the slowdown we are seeing in the nightlies though.
My doubt is that these methods are really small, so they are probably hard to sample and catch in flamegraphs.
On the other hand, in the commit diff there is really nothing apart from this change (all the rest is tests), so this is my only candidate...
There was a problem hiding this comment.
My doubt is that these methods are really small, so they are probably hard to sample and catch in flamegraphs.
If they're small but called a lot, I would statistically expect them to still show up.
| return ratio; | ||
| } | ||
| }, | ||
| YEARS_OF_CENTURY((byte) 9, "years", ChronoField.YEAR_OF_ERA, false, 12) { |
There was a problem hiding this comment.
Do we want to add these two new enums to the tests?
| @Override | ||
| long roundFloor(long utcMillis, int multiplier) { | ||
| return DateUtils.roundWeekIntervalOfWeekYear(utcMillis, multiplier); | ||
| assert multiplier == 1; |
There was a problem hiding this comment.
Not sure about this change. If we want the change as a short term one, to profile the outcome in an integrated system, it's OK, but otherwise we would better change the API.
There was a problem hiding this comment.
I'm not sure either.
Yeah, that's the intention
| long roundFloor(long utcMillis, int multiplier) { | ||
| return multiplier == 1 ? DateUtils.roundYear(utcMillis) : DateUtils.roundYearInterval(utcMillis, multiplier); | ||
| assert multiplier == 1; | ||
| return DateUtils.roundYear(utcMillis); |
There was a problem hiding this comment.
My doubt is that these methods are really small, so they are probably hard to sample and catch in flamegraphs.
If they're small but called a lot, I would statistically expect them to still show up.
|
We got a confirmation from the nightlies, this worked. |
|
Manual backport in progress |
4 participants
Try to reduce the overhead introduced by #120302
The changes are minimal, but they are on the critical execution path of date histogram calculation.