ESQL: Fix slowness in ValuesFromManyReader.estimatedRamBytesUsed#139397
ESQL: Fix slowness in ValuesFromManyReader.estimatedRamBytesUsed#139397GalLalouche merged 8 commits intoelastic:mainfrom
Conversation
GalLalouche
added
>enhancement
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @GalLalouche, I've created a changelog YAML for you. |
|
Can you please explain how and why the fix works? |
|
Do you have a corresponding after measurement to compare the gains? |
@julian-elastic Like the comment says: the |
@idegtiarenko Unfortunately, I was unable to run the rally tracks locally, as it took forever and then crashed. But running the relevant queries on my laptop was fairly consistent: this fix remove almost all of the slowness introduced by #132757. I say almost, since there are some things which are not as trivial to optimize, caused by moving to a shared context, e.g., the fact that we are iterating over |
|
Sounds good, lets see the change after the next nightly benchmark run. |
| return this.buildersAndLoaders.stream().flatMap(e -> e.values().stream()).mapToLong(bl -> bl.builder.estimatedBytes()).sum(); | ||
| long sum = 0; | ||
| for (var builderAndLoader : this.buildersAndLoaders) { | ||
| for (BlockBuilderAndLoader blockBuilderAndLoader : builderAndLoader.values()) { |
There was a problem hiding this comment.
NIT: sometimes it is also nice to avoid iterator creation in outer and especially inner loop.
Not sure if it is possible here, since inner is iterating over map.
There was a problem hiding this comment.
For the inner it is indeed impossible, but I've changed the outer to use a plain for.
At least for the rally benchmarks, this should reduce the time by almost 50%! So it should be very noticeable. |
…stic#139397) This PR fixes a slowness introduced in elastic#132757 that was first encountered on our nightly benchmarks. After much digging, and with @nik9000's invaluable help, the culprit was found to be the usage of what is apparently a very slow functional Stream in what is apparently a very hot path.
4 participants
This PR fixes a slowness introduced in #132757 that was first encountered on our nightly benchmarks. After much digging, and with @nik9000's invaluable help, the culprit was found to be the usage of what is apparently a very slow functional
Streamin what is apparently a very hot path.