[PyTorch] Use plain old function pointer for RecordFunctionCallback (reapply) by swolchok · Pull Request #49408 · pytorch/pytorch

swolchok · 2020-12-15T17:48:17Z

Stack from ghstack:

[PyTorch] Use plain old function pointer for RecordFunctionCallback (reapply) #49408 [PyTorch] Use plain old function pointer for RecordFunctionCallback (reapply)

Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback.

Differential Revision: D25563207

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

…reapply) Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25563207](https://our.internmc.facebook.com/intern/diff/D25563207/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25563207/)! [ghstack-poisoned]

…nCallback (reapply)" Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25563207](https://our.internmc.facebook.com/intern/diff/D25563207/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25563207/)! [ghstack-poisoned]

…reapply) 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: 118625284 Differential Revision: [D25563207](https://our.internmc.facebook.com/intern/diff/D25563207/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25563207/)!

malfet · 2020-12-15T18:53:28Z

aten/src/ATen/record_function.cpp

    try {
      if (is_start) {
-        ctx = rfcb.start()(rf);
+        ctx = rfcb.start() ? rfcb.start()(rf) : nullptr;


How about something like?

Suggested change

ctx = rfcb.start() ? rfcb.start()(rf) : nullptr;

if (auto& start = rcbf.start()) ctx = start(rf) else ctx = nullptr;

there's no reason to complicate the code like this; start() is an inlineable load of a single pointer

malfet · 2020-12-15T18:53:56Z

aten/src/ATen/record_function.cpp

+        if (rfcb.end()) {
+          rfcb.end()(rf, ctx.get());
+        }


Nit

Suggested change

if (rfcb.end()) {

rfcb.end()(rf, ctx.get());

}

if (auto& end = rfcb.end()) {

end(rf, ctx.get());

}

malfet · 2020-12-15T18:56:07Z

aten/src/ATen/record_function.h

-        [](const RecordFunction&, ObserverContext*) {}):
-      start_(std::move(start)),
-      end_(std::move(end)) {
+      StartCallback start,


Why not pass by reference here?(or keep std::move() when member variable is initialized

Suggested change

StartCallback start,

const StartCallback& start,

because function pointers should be passed by value, just like void* or int, for efficiency reasons. Similarly, there is no need to move them.

malfet · 2020-12-15T18:56:34Z

aten/src/ATen/record_function.h

  }

-  inline const std::function<std::unique_ptr<ObserverContext>(const RecordFunction&)>& start() const {
+  inline StartCallback start() const {


Why do we want a copy here?

Suggested change

inline StartCallback start() const {

inline const StartCallback& start() const {

because function pointers are cheap to copy, like int or void*

std::function is not just a function pointer, is it?

No, but this diff changes StartCallback to be a function pointer above.

malfet · 2020-12-15T18:58:11Z

binaries/record_function_benchmark.cc

+    at::RecordFunctionCallback::StartCallback fn =
+        [](const at::RecordFunction&) -> std::unique_ptr<at::ObserverContext> { return nullptr; }) {


Why do we need a non-trivial initializer here?

Suggested change

at::RecordFunctionCallback::StartCallback fn =

[](const at::RecordFunction&) -> std::unique_ptr<at::ObserverContext> { return nullptr; }) {

at::RecordFunctionCallback::StartCallback fn = at::RecordFunctionCallback::StartCallback()

I just left it the way the existing code had it, don't think it's expensive either way because it's a constant expression (address of a function)

facebook-github-bot · 2020-12-15T19:23:33Z

💊 CI failures summary and remediations

As of commit 62d7d46 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 4 times.

codecov · 2020-12-15T21:45:26Z

Codecov Report

Merging #49408 (62d7d46) into gh/swolchok/47/base (9234f50) will increase coverage by 0.00%.
The diff coverage is 96.87%.

@@                 Coverage Diff                  @@
##           gh/swolchok/47/base   #49408   +/-   ##
====================================================
  Coverage                80.56%   80.56%           
====================================================
  Files                     1875     1875           
  Lines                   202701   202697    -4     
====================================================
- Hits                    163309   163308    -1     
+ Misses                   39392    39389    -3

…nCallback (reapply)" Nearly every non-test callsite doesn't need to capture any variables anyway, and this saves 48 bytes per callback. Differential Revision: [D25563207](https://our.internmc.facebook.com/intern/diff/D25563207/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25563207/)! [ghstack-poisoned]

…reapply) 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 Differential Revision: [D25563207](https://our.internmc.facebook.com/intern/diff/D25563207/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25563207/)!

facebook-github-bot · 2020-12-16T05:18:15Z

This pull request has been merged in 22c6daf.

…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

swolchok requested a review from albanD as a code owner December 15, 2020 17:48

facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Dec 15, 2020

swolchok requested review from dhruvbird and malfet December 15, 2020 17:49

malfet reviewed Dec 15, 2020

View reviewed changes

malfet approved these changes Dec 15, 2020

View reviewed changes

facebook-github-bot closed this in 22c6daf Dec 16, 2020

facebook-github-bot added the Merged label Dec 16, 2020

facebook-github-bot deleted the gh/swolchok/47/head branch December 19, 2020 15:18

	ctx = rfcb.start() ? rfcb.start()(rf) : nullptr;
	if (auto& start = rcbf.start()) ctx = start(rf) else ctx = nullptr;

	inline StartCallback start() const {
	inline const StartCallback& start() const {

		at::RecordFunctionCallback::StartCallback fn =
		[](const at::RecordFunction&) -> std::unique_ptr<at::ObserverContext> { return nullptr; }) {

Conversation

swolchok commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

codecov bot commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot commented Dec 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swolchok commented Dec 15, 2020 •

edited

Loading

facebook-github-bot commented Dec 15, 2020 •

edited

Loading

codecov bot commented Dec 15, 2020 •

edited

Loading