Skip to content

Stop copying tensor memory when importing IR#10487

Closed
li-roy wants to merge 5 commits intopytorch:masterfrom
li-roy:exportnocopy
Closed

Stop copying tensor memory when importing IR#10487
li-roy wants to merge 5 commits intopytorch:masterfrom
li-roy:exportnocopy

Conversation

@li-roy
Copy link
Contributor

@li-roy li-roy commented Aug 13, 2018

No description provided.

@ezyang
Copy link
Contributor

ezyang commented Aug 15, 2018

So, I finally got a chance to look at where you are getting the shared_ptr from, and this seems a bit goofy:

    std::shared_ptr<void> retval(malloc(size), free);
    if (!std::fread(retval.get(), size, 1, fp)) {
      wrapPErrorAndThrow("Failed to read data from record");
    }
    cursor += size;
    seekToNextAlignmentBoundary();
    return std::tuple<std::shared_ptr<void>, size_t>(retval, size);

Why don't you directly return a DataPtr here, and then you don't have to do any of this faffing about, you can just gift it directly to the Storage.

facebook-github-bot
facebook-github-bot reviewed Aug 16, 2018
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.

li-roy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ailzhang
Copy link
Contributor

ailzhang commented Aug 21, 2018

@pytorchbot retest this please

@li-roy
Copy link
Contributor Author

li-roy commented Aug 27, 2018

@pytorchbot retest this please.

facebook-github-bot
facebook-github-bot reviewed Aug 27, 2018
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.

li-roy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@li-roy
Copy link
Contributor Author

li-roy commented Aug 27, 2018

@pytorchbot retest this please

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
@PenghuiCheng
Stop copying tensor memory when importing IR
74dcd61 
Summary: Pull Request resolved: pytorch#10487

Differential Revision: D9370084

Pulled By: li-roy

fbshipit-source-id: ecff1d5d7d006fd60e4f6238ee86c56ad168bfc8
@ezyang ezyang added the merged label Jun 26, 2019
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

@zdevito zdevito zdevito approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@li-roy @ezyang @ailzhang @zdevito @facebook-github-bot