-
Notifications
You must be signed in to change notification settings - Fork 61
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
Optimize remove duplicate gradients #248
Optimize remove duplicate gradients #248
Conversation
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
==========================================
- Coverage 94.18% 94.14% -0.05%
==========================================
Files 5 5
Lines 2253 2270 +17
==========================================
+ Hits 2122 2137 +15
- Misses 131 133 +2
Continue to review full report at Codecov.
|
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.
Great MR! I really like that you split it into individual commits and explained nicely what each of them does and why it is necessary. Made reviewing significantly easier!
Signed-off-by: Niels Thykier <niels@thykier.net>
This commit is mostly to enable the following commit to make improvements. It does reduce the number of duplicate getAttribute calls by a tiny bit but it is unlikely to matter in practice. Signed-off-by: Niels Thykier <niels@thykier.net>
Regex compilation is by far the most expensive part of removeDuplicateGradients. This commit reduces the pain a bit by trading "many small regexes" to "few larger regexes", which avoid some of the compilation overhead. Signed-off-by: Niels Thykier <niels@thykier.net>
This is commits enables a future optimization (but is not a notable optimization in itself). Signed-off-by: Niels Thykier <niels@thykier.net>
This is commits enables a future optimization (but is not a notable optimization in itself). Signed-off-by: Niels Thykier <niels@thykier.net>
Except for one caller, nothing cares what kind of collection is used. By migrating to a set, we can enable a future rewrite. Signed-off-by: Niels Thykier <niels@thykier.net>
This avoids calling `findReferencedElements` more than once per removeDuplicateGradients. This is good for performance as `findReferencedElements` is one of the slowest functions in scour. Signed-off-by: Niels Thykier <niels@thykier.net>
2ec1f60
to
ca2b32c
Compare
No description provided.