Skip to content

Optimize view_as_complex and view_as_real #44908

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.

Sign up for GitHub

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

Jump to bottom
Closed
peterbell10 wants to merge 1 commit into from
Closed

Optimize view_as_complex and view_as_real #44908

peterbell10 wants to merge 1 commit into from

Conversation

@peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented Sep 17, 2020 •
edited
Loading

This avoids unnecessary memory allocations in view_as_complex and view_as_real. I construct the new tensor directly with the existing storage to avoid creating a new storage object and also use DimVectors to avoid allocating for the sizes and strides. Overall, this saves about 2 us of overhead from torch.fft.fft which currently has to call view_as_real and view_as_complex for every call.

I've used this simple benchmark to measure the overhead:

In [1]: import torch
   ...: a = torch.rand(1, 2) 
   ...: ac = torch.view_as_complex(a) 
   ...: %timeit torch.view_as_real(ac)  
   ...: %timeit torch.view_as_complex(a) 
   ...: %timeit ac.real

Results before:

2.5 µs ± 62.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
2.22 µs ± 36 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
4.17 µs ± 8.76 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

and after:

1.83 µs ± 9.26 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
1.57 µs ± 7.17 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
3.47 µs ± 34.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

@mruberry mruberry added module: complex Related to complex number support in PyTorch triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 18, 2020
@mruberry mruberry requested a review from anjali411 September 18, 2020 21:01
anjali411
anjali411 reviewed Sep 18, 2020
View reviewed changes
aten/src/ATen/native/ComplexHelper.h Outdated
Copy link
Contributor

@anjali411 anjali411 Sep 18, 2020

Choose a reason for hiding this comment

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

just curious if there's a reason why back() is more preferable thank emplace_back()

Copy link
Collaborator

@mruberry mruberry Sep 18, 2020

Choose a reason for hiding this comment

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

@peterbell10 pre-allocated the size of res to avoid a (possible) reallocation

Copy link
Collaborator Author

@peterbell10 peterbell10 Sep 19, 2020

Choose a reason for hiding this comment

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

Yeah, emplace_back inserts a new element while back is just a shorthand for indexing the last element.

anjali411
anjali411 approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@anjali411 anjali411 left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

facebook-github-bot
facebook-github-bot reviewed Sep 18, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@anjali411
Copy link
Contributor

anjali411 commented Sep 21, 2020

@peterbell10 I see some internal test failures which are probably unrelated. can you rebase this PR?

@peterbell10
Optimize view_as_complex and view_as_real
a7ec88f 
Avoid unnecessary memory allocations by not constructing a new storage object
and using DimVector to store new sizes and strides.
@dr-ci
Copy link

dr-ci bot commented Sep 21, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

XLA failure

Job pytorch_xla_linux_bionic_py3_6_clang9_build is failing. Please create an issue with title prefixed by [PT_BREAK] in pytorch/xla and link to to this PR. If you have questions, please reach out to @ailzhang / @dlibenzi / @JackCaoG.

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

Since your merge base is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 2 times.

facebook-github-bot
facebook-github-bot reviewed Sep 21, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 29, 2020

@anjali411 merged this pull request in 0806c58.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@anjali411 anjali411 anjali411 approved these changes

@mruberry mruberry Awaiting requested review from mruberry

Assignees

No one assigned

Labels

Merged module: complex Related to complex number support in PyTorch open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@peterbell10 @anjali411 @facebook-github-bot @mruberry