Skip to content

[TorchTidy] Adding support for storing scalar values in profiling#81843

Closed
Gamrix wants to merge 6 commits intogh/gamrix/83/basefrom
gh/gamrix/83/head
Closed

[TorchTidy] Adding support for storing scalar values in profiling#81843
Gamrix wants to merge 6 commits intogh/gamrix/83/basefrom
gh/gamrix/83/head

Conversation

@Gamrix
Copy link
Contributor

@Gamrix Gamrix commented Jul 21, 2022

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 21, 2022

🔗 Helpful links

✅ No Failures (0 Pending)

As of commit 0ea2316 (more details on the Dr. CI page):

Expand to see more

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


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

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

Click here to manually regenerate this comment.

Gamrix added a commit that referenced this pull request Jul 21, 2022
Gamrix added a commit that referenced this pull request Jul 21, 2022
Gamrix added a commit that referenced this pull request Jul 22, 2022
Gamrix added a commit that referenced this pull request Aug 2, 2022
@Gamrix Gamrix requested a review from robieta as a code owner August 3, 2022 22:47
Gamrix added a commit that referenced this pull request Aug 3, 2022
Copy link
Contributor

@robieta robieta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Have you measured the overhead? Although it's mostly just out of curiosity; I can't imagine that another approach would be faster (or even as fast) as what you have here.

@Gamrix
Copy link
Contributor Author

Gamrix commented Aug 4, 2022

@Gamrix has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@robieta
Copy link
Contributor

robieta commented Aug 4, 2022

Overhead information: Worse than I would've expected :/

D38412482 https://our.internmc.facebook.com/intern/aibench/details/1216719728 https://our.internmc.facebook.com/intern/aibench/details/2852921225 https://our.internmc.facebook.com/intern/aibench/details/308408276 https://our.internmc.facebook.com/intern/aibench/details/3785164820 https://our.internmc.facebook.com/intern/aibench/details/1536294584

P10 P50 P90 1.2249 1.2278 1.2366 1.2244 1.2297 1.2459 1.2216 1.2269 1.2405

base of D38412482 https://our.internmc.facebook.com/intern/aibench/details/3415091160 https://our.internmc.facebook.com/intern/aibench/details/3382452677 https://our.internmc.facebook.com/intern/aibench/details/2669142924

P10 P50 P90 1.1141 1.1193 1.1369 1.1451 1.1490 1.1603 1.1221 1.1263 1.1345

That's unfortunate. How does that compare to just storing a c10::Scalar? It's not ideal, but hopefully will give us some baseline for the cost to store a simpler (albeit larger) type.

@Gamrix
Copy link
Contributor Author

Gamrix commented Aug 4, 2022

The overhead is the same (looks marginally cheaper, but very much in the margin for error on this benchmark) for Scalar, which is not surprising as you make the copy trivial, but on the other hand, you have to call ivalue.toScalar() to convert it.

https://our.internmc.facebook.com/intern/aibench/details/4076738461
https://our.internmc.facebook.com/intern/aibench/details/1092748111
https://our.internmc.facebook.com/intern/aibench/details/4144779513
1.2236 1.2272 1.2597
1.2157 1.2221 1.2310
1.1976 1.2160 1.2219

@Gamrix
Copy link
Contributor Author

Gamrix commented Aug 4, 2022

@Gamrix has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Gamrix
Copy link
Contributor Author

Gamrix commented Aug 8, 2022

@pytorchmergebot land

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 8, 2022

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: argument command: invalid choice: 'land' (choose from 'merge', 'revert', 'rebase', 'label')

usage: @pytorchbot [-h] {merge,revert,rebase,label} ...

Try @pytorchbot --help for more info.

@Gamrix
Copy link
Contributor Author

Gamrix commented Aug 8, 2022

@pytorchmergebot merge

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a merge job. Check the current status here

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Hey @Gamrix.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

facebook-github-bot pushed a commit to facebook/Ax that referenced this pull request Aug 9, 2022
Summary:
X-link: pytorch/pytorch#81843
Approved by: https://github.com/robieta

Reviewed By: ngimel

Differential Revision: D38412482

Pulled By: kit1980

fbshipit-source-id: 6e2f3c949bda6a0e2cc428a693737d1398bc68ce
facebook-github-bot pushed a commit that referenced this pull request Aug 9, 2022
…1843) (#81843)

Summary:
Pull Request resolved: #81843
Approved by: https://github.com/robieta

Test Plan:
contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/1cd1f48d9311561920c841d322a98b805e49dcc7

Original Phabricator Test Plan:
Imported from OSS

Reviewed By: ngimel

Differential Revision: D38412482

Pulled By: kit1980

fbshipit-source-id: 6e2f3c949bda6a0e2cc428a693737d1398bc68ce
@facebook-github-bot facebook-github-bot deleted the gh/gamrix/83/head branch August 12, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants