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

distributed-lazy: delete unneeded temporaries after execution #258

Merged
merged 452 commits into from
Feb 10, 2022

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Feb 7, 2022

Add-on for #148.

Should help with illinois-ceesd/mirgecom#599

@matthiasdiener matthiasdiener changed the base branch from main to distributed-v3 February 7, 2022 20:22
pytato/distributed.py Outdated Show resolved Hide resolved
@matthiasdiener matthiasdiener marked this pull request as ready for review February 8, 2022 01:18
@matthiasdiener matthiasdiener self-assigned this Feb 8, 2022
@matthiasdiener matthiasdiener changed the title Dist delete unneeded distributed-lazy: delete unneeded temporaries after execution Feb 8, 2022
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM subject to these minor fixes.

Comment on lines 612 to 621
# Keep a count on how often each input name is used,
# in order to be able to free them.
partition_input_names_refcount: Dict[str, int] = {}

for pid in pids_to_execute:
for name in partition.parts[pid].all_input_names():
if name in partition_input_names_refcount:
partition_input_names_refcount[name] += 1
else:
partition_input_names_refcount[name] = 1
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a @memoize_method on the partition. No need to recompute on every evaluation IMO. (Make sure to not modify the cached dictionary in place!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 4b592fb

@@ -668,6 +681,10 @@ def wait_for_some_recvs() -> None:
for send_req in send_requests:
send_req.Wait()

if __debug__:
for _, v in partition_input_names_refcount.items():
assert v == 0
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe also assert that the values are gone from context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 5481125

pytato/distributed.py Outdated Show resolved Hide resolved
Base automatically changed from distributed-v3 to main February 9, 2022 04:55
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! I'll aply the suggestion.

pytato/distributed.py Outdated Show resolved Hide resolved
@inducer inducer enabled auto-merge (squash) February 10, 2022 20:00
@inducer
Copy link
Owner

inducer commented Feb 10, 2022

This should be able to land after inducer/pytools#118 merges.

@matthiasdiener
Copy link
Collaborator Author

This should be able to land after inducer/pytools#118 merges.

It will need a new pytools release+conda package I guess.

@inducer
Copy link
Owner

inducer commented Feb 10, 2022

Yep, you're totally right. 🤦 Doing that now.

@inducer
Copy link
Owner

inducer commented Feb 10, 2022

https://pypi.org/project/pytools/2022.1/

@inducer inducer merged commit 1af35bb into main Feb 10, 2022
@inducer inducer deleted the dist-delete-unneeded branch February 10, 2022 22:09
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.

2 participants