[JIT] Change expect, cast on Type to return shared pointers, make isSubtypeOf accept TypePtr by wanchaol · Pull Request #9786 · pytorch/pytorch

wanchaol · 2018-07-24T21:13:49Z

Follow up task of #9584.

Commit 1:

change expect/cast to return shared pointers instead of raw pointer
isSubtypeOf accept TypePtr instead. Use x->isSubtypeOf(NumberType::get()) rather than x->isSubtypeOf(*NumberType::get())

Commit 2:

to address enable_shared_from_this pitfalls, we make the constructor private and expose the factory method to make sure user can only create it using our factory method.

facebook-github-bot

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

zdevito

This looks good to me.

wanchaol · 2018-07-25T00:14:37Z

@pytorchbot retest this please

torch/csrc/jit/type.h

apaszke

Looks good, but I'm not sure if using shared pointers everywhere is a good idea, compared to just using references. There are three issues I see:

Equality can have surprising behavior, because comparing pointers doesn't use the operator== of the underlying types, but only compares their memory location. This is ok in case of singleton types, but definitely not what we want for TensorType. It's very easy to forget about it, and the compiler won't catch this error for us. If we still want to pursue the shared_ptr path, we might want to at least have a custom subclass of it, that overrides the equality semantics.
Every XType::get() is an unnecessary refcount bump/decref, and we do those in plenty of places just to perform type checking.
They are nullable, which is usually not what you want.

torch/csrc/jit/passes/graph_fuser.cpp

torch/csrc/jit/python_ir.cpp

zdevito · 2018-07-26T18:12:02Z

We were already using type pointers before, this is just making their use consistent. There were a bunch of places where you ended up with a derived type but needed the shared pointer again.

In the future, I think it would be better to make types behave like value types (e.g. like at::Tensor or IValue), meaning that equality etc. all just works and we can pass them around as Type objects. But this would be a more invasive change that we can't make while all our other PRs are in flight.

wanchaol · 2018-07-26T18:16:00Z

@apaszke As of today in JIT, I am seeing everywhere we are using std::make_shared<Type> for Tensor, List and Tuple type, and not using reference. Exposing both raw pointer and shared pointer is not ideal since it will cause undefined behaviors if we don't do it carefully, I think that's the main motivation we want to move to a single shared_pointer semantic. So for the issues:

The only place we are using reference is when we have isSubtypeOf, and before the pr, we need to put * for most of the case like x->isSubtypeOf(*NumberType::get()) , most arguments we used is a typePtr, so it's easy for us to just accept the typePtr instead. I think there's not too much equality concern since even for isSubtypeOf we are using the object to test the equality (which use the operator==) rather than pointer.
I don't think we are singletons for different types, in fact we are creating a new type object and let a new shared pointer point to it whenever we need a type. So the lifetime of the object and the shared pointer is bounded by the usage of XType::get() .
Yes they are nullable but people are using make_shared or type::get() everywhere for different types and they are nullable too.

wanchaol · 2018-07-26T18:23:47Z

@smessmer Can you review this again? You can just review the second commit where it addressed the enable_shared_from_this pitfall.

…ypeOf accept TypePtr

…mments

facebook-github-bot

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

torch/csrc/jit/type.h

 private:
+  ListType(TypePtr elem)
+  : Type(TypeKind::ListType), elem(elem) {}
+  static const TypeKind Kind = TypeKind::ListType;


torch/csrc/jit/type.h

  }
-  bool isSubtypeOf(const Type& rhs) const override {
-    return *this == rhs || rhs.kind() == TypeKind::NumberType;
+  bool isSubtypeOf(const TypePtr rhs) const override {


…Of accept TypePtr (pytorch#9786) Summary: Follow up task of pytorch#9584. Commit 1: - change expect/cast to return shared pointers instead of raw pointer - isSubtypeOf accept TypePtr instead. Use `x->isSubtypeOf(NumberType::get())` rather than `x->isSubtypeOf(*NumberType::get())` Commit 2: - to address enable_shared_from_this pitfalls, we make the constructor private and expose the factory method to make sure user can only create it using our factory method. Pull Request resolved: pytorch#9786 Reviewed By: zdevito Differential Revision: D8980441 Pulled By: wanchaol fbshipit-source-id: e5c923fc57a701014310e77cf29985b43bb25364

wanchaol added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jul 24, 2018

wanchaol requested review from Yangqing, anderspapitto, apaszke, bddppq, colesbury, dzhulgakov, ezyang, gchanan, houseroad, jamesr66a, smessmer, soumith and zdevito as code owners July 24, 2018 21:13

facebook-github-bot reviewed Jul 24, 2018

View reviewed changes

zdevito approved these changes Jul 24, 2018

View reviewed changes

smessmer reviewed Jul 25, 2018

View reviewed changes

torch/csrc/jit/type.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/type.h Outdated

This comment was marked as off-topic.

Sign in to view

wanchaol force-pushed the cleantype branch from 81e7c7c to d50aa92 Compare July 26, 2018 05:32

apaszke reviewed Jul 26, 2018

View reviewed changes

torch/csrc/jit/passes/graph_fuser.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/python_ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

wanchaol added 2 commits July 26, 2018 11:32

Change expect, cast on TypePtr to return shared pointers, make isSubt…

defcb45

…ypeOf accept TypePtr

use factory create instead of public constructor for type, address co…

301cf0d

…mments

wanchaol force-pushed the cleantype branch from d50aa92 to 301cf0d Compare July 26, 2018 18:43

facebook-github-bot reviewed Jul 26, 2018

View reviewed changes

smessmer approved these changes Jul 26, 2018

View reviewed changes

facebook-github-bot closed this in b7b61a8 Jul 27, 2018

wanchaol deleted the cleantype branch July 27, 2018 02:46

ezyang added the merged label Jun 26, 2019

Conversation

wanchaol commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

wanchaol commented Jul 25, 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!

apaszke left a comment

Choose a reason for hiding this comment

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!

zdevito commented Jul 26, 2018

Uh oh!

wanchaol commented Jul 26, 2018

Uh oh!

wanchaol commented Jul 26, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wanchaol commented Jul 24, 2018 •

edited

Loading