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

[query] Fix memory leak in combOpFSerialized (and therefore densify) #9028

Merged
merged 1 commit into from Jun 30, 2020

Conversation

tpoterba
Copy link
Contributor

The previous implementation, while seeming to be well-abstracted at
first, actually had a rather devious property of creating agg states
for multiple classes multiple times. I'm still working on figuring out
exactly the place where our assumptions broke down, but this change
definitely fixes the problem, and simplifies the implementation by
directly using IR, instead of other compiled functions.

This problem was a symptom of a larger issue, which is that the
ownership semantics of the current aggregator system is way too complex
to be coding against regularly. This all will go away when lowering is
complete, in favor of the much simpler set of IR nodes that are used
in lowering. We may need to address this problem sooner, though.

CHANGELOG: Fixed memory leak affecting Table.annotate with scans, hl.experimental.densify, and Table.group_by / aggregate.

The previous implementation, while seeming to be well-abstracted at
first, actually had a rather devious property of creating agg states
for multiple classes multiple times. I'm still working on figuring out
*exactly* the place where our assumptions broke down, but this change
definitely fixes the problem, and simplifies the implementation by
directly using IR, instead of other compiled functions.

This problem was a symptom of a larger issue, which is that the
ownership semantics of the current aggregator system is way too complex
to be coding against regularly. This all will go away when lowering is
complete, in favor of the *much* simpler set of IR nodes that are used
in lowering. We may need to address this problem sooner, though.

CHANGELOG: Fixed memory leak affecting `Table.annotate` with scans, `hl.experimental.densify`, and `Table.group_by` / `aggregate`.
Copy link
Collaborator

@chrisvittal chrisvittal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for coming up with a solution to this.

@danking danking merged commit 804d2b2 into hail-is:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants