Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve nested loop operator #5276

Merged
merged 2 commits into from Oct 8, 2020

Conversation

pettyjamesm
Copy link
Member

Also adds a constructor to the Page class for when no blocks are passed as arguments.

@cla-bot cla-bot bot added the cla-signed label Sep 23, 2020
@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 2 times, most recently from 09bdec1 to c416dcb Compare September 23, 2020 15:25
@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 2 times, most recently from 2fad348 to c16fb21 Compare September 23, 2020 22:31
@martint martint requested a review from dain September 24, 2020 04:06
@pettyjamesm pettyjamesm marked this pull request as draft September 24, 2020 16:51
@pettyjamesm
Copy link
Member Author

Marking as WIP - I think I have a much better approach

@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 2 times, most recently from 5c80b06 to 0f6108d Compare September 24, 2020 17:40
@dain dain requested a review from sopel39 September 24, 2020 18:23
@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 2 times, most recently from e3e986f to b895710 Compare September 24, 2020 21:47
@sopel39
Copy link
Member

sopel39 commented Sep 25, 2020

@pettyjamesm which of the optimizations in this PR are valid when we assume that cross join cannot do build pruning (which it cant)? Can the PR be simplified?

@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 3 times, most recently from adba160 to e5ff6ac Compare September 25, 2020 13:30
@pettyjamesm
Copy link
Member Author

@pettyjamesm which of the optimizations in this PR are valid when we assume that cross join cannot do build pruning (which it cant)? Can the PR be simplified?

Good to know and thanks for the review. I've stripped out the parts that aren't necessary when we can safely assume no pruning actually happens. Should be ready for another review now.

@pettyjamesm pettyjamesm marked this pull request as ready for review September 25, 2020 13:40
@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 2 times, most recently from a3f9ecf to bf9ac12 Compare October 5, 2020 18:13
@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 3 times, most recently from bb27cc7 to 40a966a Compare October 7, 2020 12:24
Avoids varargs empty array creation per call for these usage sites
@pettyjamesm pettyjamesm force-pushed the improve-nested-loop-operator branch 2 times, most recently from b978035 to dd1dbfd Compare October 8, 2020 13:01
- Modifies NestedLoopJoinPagesBuilder to combine empty pages (aka:
positionCount only pages) when the build side of the nested loop
join is empty
- Handles the case where either probe or build side outputs are empty
and position counts are fewer by emitting the same page repeatedly,
avoiding unnecessary per-iteration allocations
- Reduces the amount of block array copies made in the standard case
by reusing a block buffer to build each page and letting the Page
constructor clone it (ie: 1/2 as many allocations)
- Nullifies the output iterator as well as the probe page when finished
iterating through it to make the referenced pages elligible for GC. Also
nullifies more fields when the operator is closed for the same reason.
@martint martint merged commit 7f303fb into trinodb:master Oct 8, 2020
@pettyjamesm pettyjamesm deleted the improve-nested-loop-operator branch October 9, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants