[REBASE] Merge THTensor into TensorImpl by gchanan · Pull Request #10479 · pytorch/pytorch

gchanan · 2018-08-13T19:04:56Z

No description provided.

gchanan · 2018-08-13T19:05:33Z

This is a direct rebase/merge of #10421; I have not yet reviewed this code.

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

facebook-github-bot

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

Summary: Fix leaking of Storages (not StorageImpls) Reviewed By: li-roy Differential Revision: D9349824 fbshipit-source-id: d492d7d62be393cbc5955440278bd605e04767a4

gchanan · 2018-08-15T21:47:30Z

This should be ready to review.

Some notes:

There are still some member variables that are publicly accessible in TensorImpl; we need to rework TH to remove these.
I removed unsafeGetTH because there are no longer THTensors. There's a little complexity with this because the closest equivalent (unsafeGetTensorImpl) is a bit confusing for Variable::Impls since a Variable::Impl is both "is-a" and "has-a" TensorImpl. The function returns the "is-a", and you have to know what you are doing to extract the data. This seems more-or-less fine, but may confuse external users (hopefully there aren't any). And this shouldn't be a long term problem, because we are planning to get rid of the "has-a" case.
I included a StorageImpl * accessor in TensorImpl because it makes things convenient temporarily. This should be changed to use intrusive_ptr.
Because of Variable::Impl (and sometimes, SparseTensorImpl), a bunch of these functions are virtual when they don't really have to be. This unfortunately makes a bunch of functions in TH virtual as well, but this should only be temporary.
For the familiar reason of Variable::Impl, this doesn't merge the size data member usage in TensorImpl and SparseTensorImpl.

aten/src/ATen/SparseTensorImpl.cpp

+  return size_[d];
+}
+int64_t SparseTensorImpl::stride(int64_t d) const {
+  AT_ERROR("sparse tensors do not have strides");


aten/src/ATen/TensorImpl.cpp

+    auto type = &globalContext().getType(tensorTypeIdToBackend(type_id), scalar_type);
+    auto storage = type->storage(true);
+    storage_ = storage->pImpl();
+    storage_->retain();


aten/src/ATen/TensorImpl.cpp

+  if (storage_) {
+    storage_->release();
+    storage_ = nullptr;
+  }


aten/src/ATen/TensorImpl.cpp

-      tensor->release();
-      tensor = nullptr;
+  if (storage_) {
+    storage_->release();


aten/src/TH/generic/THTensor.h

 #define TH_GENERIC_FILE "generic/THTensor.h"
 #else

-/* a la lua? dim, storageoffset, ...  et les methodes ? */


facebook-github-bot

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

facebook-github-bot

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

Summary: Pull Request resolved: pytorch/pytorch#10479 Differential Revision: D9315800 Pulled By: gchanan fbshipit-source-id: b13ef0de3342600b02b54e0700eb02021a9d1a9e

gchanan and others added 4 commits August 10, 2018 14:43

Merge THTensor into TensorImpl

cd66421

More merge

0e350c6

more merges

28013aa

Merge remote-tracking branch 'origin/master' into tensor3a

2da3cf0

gchanan requested review from apaszke, colesbury, ezyang, soumith and zdevito as code owners August 13, 2018 19:04

gchanan added 11 commits August 13, 2018 12:21

Fix merge issues.

617554b

More merge.

8eb045c

More merge correction.

7b0adb5

More merging.

07864da

Merge remote-tracking branch 'origin/master' into tensor3a

10626fe

More merge stuff.

55b13df

Properly set is_variable_, return from maybe_zero_dim.

6ad2879

Get rid of try/catch.

7dcd93b

Restore TensorImpl destructor.

d7c357c

Minimize diff.

4d6e727

Clean up THTensor defines.

ef14f9b

facebook-github-bot reviewed Aug 14, 2018

View reviewed changes

gchanan added 3 commits August 14, 2018 08:44

Add back is_variable.

1c6e777

Pass is_variable.

ad74616

Should be working.

477aba5

ezyang mentioned this pull request Aug 15, 2018

Merge THTensor into TensorImpl #10421

Closed

Fix extra {

a5138a4

facebook-github-bot reviewed Aug 15, 2018

View reviewed changes

gchanan added 2 commits August 15, 2018 08:29

Use scalar_type in TensorImpl.

207bb28

Fix sparse.

2d5e327

Use int64_t for storage_offset.

28f0184

facebook-github-bot reviewed Aug 15, 2018

View reviewed changes

Update UndefinedTensor.

551d392

facebook-github-bot reviewed Aug 15, 2018

View reviewed changes

Fix up undefined_tensor_test.

c0eb8df

facebook-github-bot reviewed Aug 15, 2018

View reviewed changes

gchanan added 6 commits August 15, 2018 12:59

Small fixups.

4273e2b

Fix leaking of Storages (not StorageImpls)

cbcdcd3

Summary: Fix leaking of Storages (not StorageImpls) Reviewed By: li-roy Differential Revision: D9349824 fbshipit-source-id: d492d7d62be393cbc5955440278bd605e04767a4

remove constructor that doesn't make sense.

944d854

Clean up constructors more.

10b4a8e

Fix up.

b13ceda

Merge remote-tracking branch 'gchanan/export-D9349824' into tensor3a_

2677739

ezyang reviewed Aug 15, 2018

View reviewed changes

aten/src/ATen/SparseTensorImpl.cpp

return size_[d];

}

int64_t SparseTensorImpl::stride(int64_t d) const {

AT_ERROR("sparse tensors do not have strides");

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 15, 2018

View reviewed changes

aten/src/ATen/TensorImpl.cpp

auto type = &globalContext().getType(tensorTypeIdToBackend(type_id), scalar_type);

auto storage = type->storage(true);

storage_ = storage->pImpl();

storage_->retain();

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 15, 2018

View reviewed changes

aten/src/ATen/TensorImpl.cpp

if (storage_) {

storage_->release();

storage_ = nullptr;

}

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 15, 2018

View reviewed changes

aten/src/ATen/TensorImpl.cpp

tensor->release();

tensor = nullptr;

if (storage_) {

storage_->release();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang reviewed Aug 15, 2018

View reviewed changes

aten/src/TH/generic/THTensor.h

#define TH_GENERIC_FILE "generic/THTensor.h"

#else

/* a la lua? dim, storageoffset, ... et les methodes ? */

This comment was marked as off-topic.

Sign in to view

ezyang approved these changes Aug 15, 2018

View reviewed changes

gchanan added 2 commits August 15, 2018 15:13

properly set nulltpr_.

dafc208

Add back French comment.

78113c7

facebook-github-bot reviewed Aug 15, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into tensor3a_

26e44a2

facebook-github-bot reviewed Aug 16, 2018

View reviewed changes

facebook-github-bot closed this in 00f2731 Aug 16, 2018

zdevito pushed a commit to zdevito/ATen that referenced this pull request Aug 16, 2018

Merge THTensor into TensorImpl

27743da

Summary: Pull Request resolved: pytorch/pytorch#10479 Differential Revision: D9315800 Pulled By: gchanan fbshipit-source-id: b13ef0de3342600b02b54e0700eb02021a9d1a9e

ezyang added the merged label Jun 26, 2019

Conversation

gchanan commented Aug 13, 2018

Uh oh!

gchanan commented Aug 13, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gchanan commented Aug 15, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants