Perf: avoiding epoch.__eq__#224
Open
sneakers-the-rat wants to merge 2 commits into
Open
Conversation
Comment on lines
106
to
+107
| # if we create epochs out of order | ||
| self._clock = count( | ||
| max([this_epoch[0].epoch, *[ep[0].epoch for ep in self._epochs], *self._epoch_log]) | ||
| + 1 | ||
| ) | ||
| self._last_epoch = max(self._last_epoch, this_epoch[0].epoch) |
Collaborator
Author
There was a problem hiding this comment.
avoiding needing to iterate through all epochs to find the max - we only need to consider the last epoch we've created and have an integer rather than an iterator.
Comment on lines
-107
to
-110
| if this_epoch in self._epochs: | ||
| raise EpochExistsError(f"Epoch {this_epoch} is already scheduled") | ||
| elif this_epoch in self._epoch_log: | ||
| raise EpochCompletedError(f"Epoch {this_epoch} has already been completed!") |
Collaborator
Author
There was a problem hiding this comment.
avoid needing to use eq when searching through the epoch log (deque, epochs is a dict and key lookup is O(1)) - we only need to check this when we are passed an epoch explicitly. if we are just making 'the next one' then we can trust the internal counter (someone can come along and purposely fuck this up that that's on them lol)
Comment on lines
+416
to
+417
| if isinstance(epoch, Epoch): | ||
| ep = epoch |
Collaborator
Author
There was a problem hiding this comment.
simple reordering
sneakers-the-rat
force-pushed
the
perf-epoch-eq
branch
from
1a68d7b to
0321098
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
doing some simple profiling and perf improvements,
Epoch.__eq__was taking an astonishingly long time, 6-7% of runtime for the kitchen sink with the async runner in 1000 process calls.turns out that can totally be eliminated in normal runs by just reordering things s.t. we don't need to call
==when we are given an epoch. comments inline📚 Documentation preview 📚: https://noob--224.org.readthedocs.build/en/224/