Unifying Object Protocol in the Stack

Development pre-RFC
#12

This is what was proposed in A1(force type index of all the sub-classes to be in a range)

Hmmm…While euler tour of tree exactly means “force some index to be contiguous within a tree”, I am not saying that is type index. What I am trying to say is that we can map those immutable type_index to a mutable index using an array called euler_order_range.

In other words, we allow type_index to be arbitrary, but enforce $euler_order_range(type_index(child)) \subseteq euler_order_range(type_index(parent))$, where euler_order_range can be implemented as a global array. Every time after we load a new module, we can re-calculate this array.

Is that this solution much like A2?

#13

I am not sure if I understand correctly. So the basic reason that we need A1 + A2 is that A2 alone seems slow for 2 reasons.

  1. access global structure
  2. might need a global lock

For 2), I am not sure if we really need a lock, or we can ask developers to ensure modules are loaded sequentially. For 1), I am not sure this would cause slowdown…

#14

Interesting, I like your idea about having the new indirection. Right now I am just implementing a parent tree traversal for checks given that A1 is already fast for most cases.

The main reason for lock is to make things thread-safe and simple(because we are using vector). It could be possible that lock was not needed(but atomic is still needed to guarantee the effect being observed by other thread) if we ensure the update won’t happen in execution phase.

#15

Given the fact (?) that loading modules is rare, I think we could either enforce loading to be sequential and loading and reading shouldn’t happen concurrently; or we can have a reader-writer lock

#16

Actually dmlc registry is not quite thread safe either

#17

I can implement the idea that I proposed. Could be done in a single method within 100 lines without recursion.

#18

OK, I will try to send in a draft PR, once we land the first version, perhaps we can then improve it possibly with your suggestion, how does that sound? Implementation wise there is no breaking changes to interface. Now that I think about it, if we can remove locking, the cost of parent traversal might be OK because most class does not have a deep hierachy

#19

Sure. My code is available here. Should be correct as I tested against random generated tree of 10k nodes.

1 Like
#20

I prefer A2 as its design is more clean and systematic. If we find performance issues later, we can then add back A1 for fast pass.

#21

Given that we have conclusion (?) over the previous two topics, let’s start another topic.

Is this part of our topic to make everything in the node system POD-C, so that we can use them cross C ABI?

#22

This sis something that we can keep in mind and gradually do. But yes it would be an interesting goal especially for FFI and DLL boundaries

#23

We can have a global registry and node system across tvm, mx, dgl, etc

#24

There are actually more to think about:

  1. Subclass NDArrays
  2. Sparse format
  3. relay.Constant silently assumes the constant is NDArray, which is not convenient for downstream projects. What about moving to Object?
#25

I created a formal RFC given most of the technical decisions are cleared. https://github.com/dmlc/tvm/issues/4116

#26

Hi Tianqi,

We tried to upgrade to the latest object protocol, and everything seems pretty smooth. And also, one great benefit noted is that we have more control over the objects, like changing deleters, etc.

Just out of curiosity, the definition of NodeRef is still standalone, which is actually different from simple aliases like Node, or NodePtr. Is this inconsistency by design, or just legacy?

#27

I think that is just a legacy item that we need to remove and then alias ObjectRef as NodeRef

#28

As we know, TVMValue is a superset of TVM object, which may be TVM object or other POD type (https://github.com/apache/incubator-tvm/blob/e91cc5aba8f99ffe216a6188edf6818e1b87237f/include/tvm/runtime/c_runtime_api.h#L150).

I was just wondering if it is possible to unify those POD types with TVM object - this would be helpful when we want to move POD types into TVM containers.

#29

we are moving towards that. There are a few options:

Option1 is to introduce Object counterpart for each pod type(int float) and allow automatic conversions between the object and the pod values. This is a bit like java

Option2 is to wrap TVMRetValue in an object, this is a bit strange though

Option 3 is to introduce a Value concept, eg make TVMRetValue a first pass thing. There are trade offs in this though because now value and objects needs two words(one for code and one for ptr) and that also means quite drastic changes to FFI

Right now I feel option1 is perhaps the best

#30

I am not sure if I understand Option 1, does it mean that we introduce Integer, Float classes like Java? The problem is that it incurs some overhead in unpadded atomic ref counting, which is actually not necessary for immutable POD types.

Option 2, if I understand correctly, means to add a new object subclass that contains TVMTypeCode and TVMValue. I was thinking about this previously, but I didn’t think it actually addressed any issue.

The fundamental reason that I asked about this is that it seems to me that TVMTypeCode and type_index are basically very similar things, so I am wondering if those two things can be merged.

#31

if we use these objects for every computation, then we are in bad shape.On the other hand, the need of object is mainly for the cases of container objects(e.g) and I believe they are not as bad for argument passing. I have thought about making type_index and TVMTypeCode consistent, I think eventually they should be as consistent as possible(e.g. use the typecode id for integer container’s type_index), however, seems we are also fine with the current setup.