Use a u64 counter for autodiff NodeIDs #843
Merged
Conversation
This file contains 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
I measure an approximate 13.6% performance boost in our crate from this change.
| } | ||
|
|
||
| impl NodeID { | ||
| /// Create a unique [node id](NodeID). | ||
| pub fn new() -> Self { | ||
| static COUNTER: AtomicU64 = AtomicU64::new(0); |
There was a problem hiding this comment.
Yeah, I thought about using u64 for autodiff IDs; it makes more sense for that use case. However, for extremely long training, it might become a problem (overflow). Do we know what behavior occurs in that case? I think it would be better if it were to trigger a panic. In that case, the training would crash and could be restarted from a checkpoint, instead of potentially corrupting the weights without understanding why. Of course, it would have to be an exceptionally long training period, and if it becomes a problem, we could consider using u128 (two u64) in that scenario.
nathanielsimard
approved these changes
Oct 4, 2023
dae
added a commit
to open-spaced-repetition/fsrs-rs
that referenced
this pull request
Oct 4, 2023
L-M-Sherlock
pushed a commit
to open-spaced-repetition/fsrs-rs
that referenced
this pull request
Oct 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I measure an approximate 13.6% performance boost in our crate from this change.
Pull Request Template
Checklist
run-checksscript has been executed.Related Issues/PRs
#839
Provide links to relevant issues and dependent PRs.
Changes
Summarize the problem being addressed and your solution.
Testing
Describe how these changes have been tested.