8000 Garbage collection for naive scheduler by fjosw · Pull Request #101 · aportelli/Hadrons · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Garbage collection for naive scheduler #101

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
Merged
merged 3 commits into from
Jan 24, 2023
Merged

Garbage collection for naive scheduler #101

merged 3 commits into from
Jan 24, 2023

Conversation

fjosw
Copy link
Contributor
@fjosw fjosw commented Jan 11, 2023

This PR adds an additional garbage collection step after every module, ensuring that all temporary objects are freed. This step is necessary for the naive scheduler which skips the memory profiling step (which is very costly for some GPU workflows) and thus does not know about the temporary objects before running the individual modules. When using the genetic scheduler the additional step does nothing.

@fjosw fjosw requested a review from aportelli as a code owner January 11, 2023 15:01
@aportelli
Copy link
Owner

Hi @fjosw that looks good thanks. freeObject is safe as it does nothing on an already destroyed object.
It would be good to test that on some kind of production job (or a mockup).

@fjosw
Copy link
Contributor Author
fjosw commented Jan 11, 2023 •
edited
Loading

I agree that the additional check for env().hasCreatedObject(a) is redundant but I felt it helps with readability. I can also remove it if you want.

The code works with a simple example on my laptop. @felixerben is now trying his workflow on tursa.

@aportelli
Copy link
Owner

That was just a general comment, but yes I think we could remove env().hasCreatedObject(a) to make the code simpler. I'll wait for @felixerben 8000 to confirm that some sort of realistic workflow works with that. Thanks again!

@felixerben
Copy link
Contributor

Thanks a lot Fabian. I have a job in the queue with those changes, let's see how it performs!

@aportelli aportelli merged commit d042678 into aportelli:develop Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aportelli aportelli Awaiting requested review from aportelli aportelli is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fjosw @aportelli @felixerben
0