box: fix drop_while integration with index:pairs#10736
Conversation
Buristan
left a comment
There was a problem hiding this comment.
Hi, Andrey!
Thanks for the patchset!
I will proceed with the review per-patch below.
Side note: I'd rather add the benchmark in separate PR to compare results before submodule bump and after, but this is insignificant due to our unstable results since we are not using only one specific machine for it.
[PATCH 1/2] luafun: bump submodule
LGTM, except a few nits regarding the commit message and the comments.
with the same
statepassed), olddrop_whileimplementation didn't
Typo: s/old/the old/
Note that there are still methods that doesn't work correctly with
Typo: s/doesn't/don't/
test/box-luatest/gh_6403_space_pairs_luafun_integration_test.lua
Outdated
Show resolved
Hide resolved
test/box-luatest/gh_6403_space_pairs_luafun_integration_test.lua
Outdated
Show resolved
Hide resolved
Buristan
left a comment
There was a problem hiding this comment.
[PATCH 2/2] perf: introduce perf test for luafun
Thanks for the patch!
I'm glad to see that we are adding new benchmarks:)!
Please consider my comments below.
You've mentioned several special cases in luafun/luafun@90967c98. Are they insingnificant (or too syntetic) to be added?
203093d to
406b477
Compare
The commit bumps luafun to the new version with a bunch of bugfixes: * Now `chain` works correctly with iterators without `param`. * Now `drop_while` supports stateful iterators. * The module is populated with missing `maximum_by` alias of `max_by`. * Now `nth` and `length` work correctly with other luafun iterators. Since our index iterators are stateful (can return different values with the same `state` passed), the old `drop_while` implementation didn't work well with them - it was skipping an extra element. The bump resolves this issue. Note that there are still methods that don't work correctly with `index:pairs` - `cycle`, `head` and `is_null`. Closes #6403 NO_DOC=bugfix
When fixing `drop_while` function in `luafun` submodule, we wrote a benchmark to choose the most efficient solution - let's put it to our perf test suite. The commit creates a new perf test for `luafun` module and puts there above-mentioned benchmark. NO_TEST=perftest NO_CHANGELOG=perftest NO_DOC=perftest
406b477 to
51e1cf8
Compare
|
Added other cases from original benchmark - |
Buristan
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
LGTM!
I asked Alexander to take one more look at the PR since new benchmarks were added.
Totktonada
left a comment
There was a problem hiding this comment.
I've no objections. Thanks for the updates!
|
Who will merge the patch? |
|
In my understanding of the process it is in the responsibility of @sergepetrenko. |
Since our index iterators are stateful (can return different values with the same state passed), old luafun drop_while implementation didn't work well with them - it was skipping an extra element. The PR bumps luafun and resolves this issue.
Closes #6403