[PyTorch] Use plain old function pointer for RecordFunctionCallback #48629
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
Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) [ghstack-poisoned]
facebook-github-bot
added
cla signed
oncall: jit
This was referenced Dec 1, 2020
swolchok
added a commit
that referenced
this pull request
Dec 1, 2020
Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) ghstack-source-id: 117508738 Pull Request resolved: #48629
💊 CI failures summary and remediationsAs of commit 74229e0 (more details on the Dr. CI page):
🕵️ 15 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
…nCallback" Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) [ghstack-poisoned]
swolchok
added a commit
that referenced
this pull request
Dec 4, 2020
Pull Request resolved: #48629 Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. ghstack-source-id: 117893077 Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25135415/)!
dhruvbird
reviewed
Dec 8, 2020
Comment on lines
+258
to
+260
| if (rfcb.end()) { | ||
| rfcb.end()(rf, ctx.get()); | ||
| } |
There was a problem hiding this comment.
Is there a need to cache the return value from rfcb.end() to prevent a race condition where the value may change in between or will that not happen by construction?
|
It would be good to know benchmarking results of before and after. |
|
Overall looks good to me. I assume that the loss of convenience (when testing) and the reliance on global/statics is something that is acceptable to everyone for the reduction in object size. I've left some comments on https://www.internalfb.com/diff/D25135415 for one of the fb-specific tests. Is there a benchmark result for pre/post this change? |
dhruvbird
approved these changes
Dec 8, 2020
…nCallback" Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) [ghstack-poisoned]
swolchok
added a commit
that referenced
this pull request
Dec 9, 2020
Pull Request resolved: #48629 Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. ghstack-source-id: 118145423 Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25135415/)!
|
@kimishpatel The benchmark results are in the test plan in the FB specific diff. |
…nCallback" Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) [ghstack-poisoned]
swolchok
added a commit
that referenced
this pull request
Dec 9, 2020
Pull Request resolved: #48629 Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. ghstack-source-id: 118226115 Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25135415/)!
…nCallback" Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) [ghstack-poisoned]
swolchok
added a commit
that referenced
this pull request
Dec 14, 2020
Pull Request resolved: #48629 Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. ghstack-source-id: 118568240 Differential Revision: [D25135415](https://our.internmc.facebook.com/intern/diff/D25135415/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25135415/)!
|
Unlanding. This appears to have broken numerous builds. Note that the Github CI also shows numerous failures. Example snippet: |
|
This pull request has been reverted by 25bc906. |
|
This pull request has been merged in 7e23ee1. |
|
@swolchok why this change was landed when it clearly introduced 15 new build failures? |
I've been trying to land it for a while, and I'm fairly sure I saw a clean CI run previously. Must have come in during rebasing. In particular, I would expect internal tests to have caught test_misc.cpp breaks and I'm confused that they didn't. |
|
Looks like the test_misc changes in particular may have failed because of a difference between C++14 and C++17. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Dec 16, 2020
…reapply) (#49408) Summary: Pull Request resolved: #49408 Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. ghstack-source-id: 118665808 Test Plan: Wait for GitHub CI since we had C++14-specific issues with this one in previous PR #48629 Reviewed By: malfet Differential Revision: D25563207 fbshipit-source-id: 6a2831205917d465f8248ca37429ba2428d5626d
hwangdeyu
pushed a commit
to hwangdeyu/pytorch
that referenced
this pull request
Jan 6, 2021
…reapply) (pytorch#49408) Summary: Pull Request resolved: pytorch#49408 Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. ghstack-source-id: 118665808 Test Plan: Wait for GitHub CI since we had C++14-specific issues with this one in previous PR pytorch#48629 Reviewed By: malfet Differential Revision: D25563207 fbshipit-source-id: 6a2831205917d465f8248ca37429ba2428d5626d
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.
Stack from ghstack:
Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback.
Differential Revision: D25135415