Repository navigation
Faster synchronization Fence primitive
#1773
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
cb2d063
try faster synchronization
awni 151a2a0
non-functioning kernel
awni 2ebe605
try alternative fence
awni a73faaa
cleanup barrier
awni 2de0241
get rid of event_fence
awni 2f40d95
update benchmarks
awni aca0721
doc string in metal fence
awni File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import time | ||
|
|
||
| import mlx.core as mx | ||
|
|
||
| world = mx.distributed.init() | ||
|
|
||
| a = mx.ones((5, 5), mx.int32) | ||
| its = 10 | ||
| its_per_eval = 100 | ||
|
|
||
|
|
||
| def fn(x): | ||
| for _ in range(its_per_eval): | ||
| x = mx.distributed.all_sum(x) | ||
| x = x - 1 | ||
| return x | ||
|
|
||
|
|
||
| # warmup | ||
| for _ in range(5): | ||
| x = fn(a) | ||
| assert mx.array_equal(x, mx.ones_like(x)) | ||
|
|
||
| tic = time.perf_counter() | ||
|
|
||
| for _ in range(its): | ||
| x = fn(a) | ||
| mx.eval(x) | ||
|
|
||
| toc = time.perf_counter() | ||
| ms = 1000 * (toc - tic) / (its * its_per_eval) | ||
| print(f"Time per iteration {ms:.6f} (ms)") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am kinda confused by this wait here. In the case where we don't use the
Fenceat all when would theevent_fencebe updated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying an alternative that doesn't require this fence cause I don't like it. But basically you can always wait on a fence. The wait will wait for any preceding calls to update. So if you never update it the wait it is essentially a no-op.
This fence is used to ensure no kernels start before the GPU signal kernel is done. So we update this fence when we signal from the GPU and then any command encoder that waits after that will wait for that update to finish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I pushed a change to get rid of this that should (and seems) to work, so you can disregard all the previous stuff with
event_fence.Basically it requires modifying the call to
wait_gputo take an array that we want to be sure is ready before any future kernels that depend on it run. It reuses our existing synchronization machinery (barriers + fences) and is nice in that it only encoders which actually depend on the output will wait for it.I had to add a way to
register_output_arraysince it's not actually part of a kernel.. but I think it's cleaner/ more efficient / and doesn't require this randomstream_eventwhich was very icky.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a horrible hidden API. I assumed wait without update is a deadlock hence the confusion before.
Yeah, so much better! Also only waiting on things that matter rather than everything on the whole stream. Plus avoid waiting on two fences which could have been the case before.