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.

Sign up for GitHub

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

Jump to bottom

Avoid cloning values during reduce #347

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Avoid cloning values during reduce #347

wants to merge 2 commits into from

Conversation

petrosagg
Copy link
Contributor

This PR can be merged as-is or only its first commit depending on the type of trade-off we want to do:

  • The first removes the need for cloning the output buffer during reduce but in order to do so it must allocate a new buffer for references every time compute is invoked. The expectation is that in most cases it makes sense to do one allocation for references than copy potentially complicated value types.
  • The second commit brings back the benefits of also reusing the buffer of references but at the cost of code complexity. In order to make the lifetimes work the raw parts of the buffer need to be stored in the struct. The commit includes a description and a safety argument.

@petrosagg
reduce_core: present referenced data to logic function
f0dc750 
`reduce_core` was previously forced to clone the output data before
presenting it to the logic function for inspection.

Simply annotating the type with a reference was not possible due to the
buffer being held in the same struct as some of the data it contained,
making it a self-referencial struct which the borrow checker doesn't
allow.

However, the only reason this buffer was held in the same struct was to
reuse its allocation. This patch therefore creates a brand new vector
every time we need to process data that contains references instead of
cloned data. The expectation is that in the general case it's better to
allocate a vector of references than clone a bunch of values.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
@petrosagg
reduce: reuse the output buffer allocation
3d625d2 
This patch brings back the benefits of re-using the output buffer
allocation at the cost of some unsafe code reasoning.

Since the buffer is always cleared after each use we can simply store
its type erased pointer and temporarily bring it back to its Vec form
whenever there is some processing to do.

Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@petrosagg