[VirtualMachine] fix raw pointer using by VirtualMachine by vvchernov · Pull Request #9980 · apache/tvm

vvchernov · 2022-01-19T16:31:06Z

The issue was observed during generation of VirtulaMachine Module on python side and using it on c++ one.
Work case: when we work in one python session the lib (exec) object is generated and used for VirtualMachine compilation. Inside VM the raw pointer to the lib (exec) is copied. Due to the lib object is alive along the session the VM object can use the pointer and execute different methods like "set_input", "invoke" and so on.
Unworkable case: if we use python session for generation VM and try to use it on c++ side it does not work. The reason is the lib object died after the session is closed. Thus VM uses the pointer to the spoiled object and it leads to failure.

To fix this problem new field in VM (ObjectPtr) is used instead of Executable* exec_. ObjectPtr object is constructed by GetObjectPtr() and used as before.
There is shorter workaround: use this->IncRef() in executable.cc. But it looks no so good and I followed @tqchen advice

@jwfromm @tmoreau89 please see

tqchen · 2022-01-19T17:36:32Z

include/tvm/runtime/object.h

  ObjectPtr(const ObjectPtr<U>& other)  // NOLINT(*)
      : ObjectPtr(other.data_) {
-    static_assert(std::is_base_of<T, U>::value,
-                  "can only assign of child class ObjectPtr to parent");


Likely we want to keep this static assert

Agree with TQ this should not be remove

jroesch · 2022-01-19T18:15:57Z

include/tvm/runtime/vm/vm.h

  ObjectRef return_register_;
  /*! \brief The executable the VM will operate on. */
-  Executable* exec_;
+  ObjectPtr<Executable> exec_;


If you get Executable an ObjectRef wrapper like the rest of the code base you can then use the down casting machinery built on-top of ObjectRef. It might make working with this type easier.

I have gotten idea, I will try

vvchernov · 2022-01-21T17:23:45Z

Hello @tqchen @jroesch Are there any additional comments, questions?

tqchen · 2022-01-22T01:32:30Z

include/tvm/runtime/vm/vm.h

   * \param exec The executable.
   */
-  virtual void LoadExecutable(Executable* exec);
+  virtual void LoadExecutable(const ObjectRef& exec);


consider change to ObjectPtr<Executable> which can be constructed via https://github.com/apache/tvm/blob/main/include/tvm/runtime/object.h#L877

It was advice from @jroesch to use ObjectRef. And it seems the simplest way from possible ones. I agree that using of ObjectPtr is preferable but to get it correctly is not so simple task (see below).

tqchen · 2022-01-22T01:33:17Z

src/runtime/vm/executable.cc

    return PackedFunc([sptr_to_self, this](TVMArgs args, TVMRetValue* rv) {
      auto vm = make_object<VirtualMachine>();
-      vm->LoadExecutable(this);
+      vm->LoadExecutable(ObjectRef(sptr_to_self));


GetObjectPtr(this);

As I know sptr_to_self is already ObjectPtr<Object>, if it is immediatly ObjectPtr<Executable> or can be simply converted to ObjectPtr<Executable> it will be nice, but it is not so. Unfortunately your approach is bad due to GetObjectPtr(...) creates new ObjectPtr<> object which contains copy of raw pointer. There is no any connection between origin and new ObjectPtr. It means that if origin object was released (e.g. when python session where it was generated is closed) we will have pointer to broken object and can not use it. just my PR solves this problem, but your approach does not as I think.

Sorry for the delayed reply. sptr_to_self actually is guanranteed to point to this in this particular case. So actually GetObjectPtr<Executable>(...) should work in this case. This is mainly because ObjectPtr is an intrusive pointer that directly leverages the object data structure.

To confirm, we can add ICHECK(sptr_to_self.get() == this);

tqchen · 2022-01-22T01:34:04Z

cc @YuchenJin @yongwww since this is VM related

vvchernov · 2022-01-26T12:43:52Z

Hello @tqchen @jroesch @yongwww @YuchenJin, any questions? May be we merge this PR?

tmoreau89 · 2022-01-27T01:33:58Z

@vvchernov there seems to be a CI issue - from the looks of it it might be flaky - can you re-trigger?

jwfromm · 2022-01-27T18:12:48Z

@areusch or @driazati is there a way around the CI error this PR is hitting? Seems like the doc generation is being flaky.

driazati · 2022-01-27T19:56:02Z

Docs were broken for a while earlier, @vvchernov could you rebase (git fetch origin && git rebase origin/main) one more time? The latest commit (fa317ed) should have gotten everything back in working order

vvchernov · 2022-01-28T10:59:44Z

Thanks @driazati ! It works

tmoreau89 · 2022-01-28T16:23:19Z

@driazati does this error seem like a flaky issue to you?

worker 'gw3' crashed while running 'tests/python/contrib/test_ethosu/test_codegen.py::test_mean[ifm_shape8-axis8-False-False-ethos-u55-256]'

tmoreau89 · 2022-01-28T16:23:51Z

At first sight the CI error seems completely unrelated to the PR...

tqchen · 2022-01-28T18:47:22Z

@vvchernov would be great if you can confirm my latest comment, which should be a better fix

Sorry for the delayed reply. sptr_to_self actually is guanranteed to point to this in this particular case. So actually GetObjectPtr<Executable>(...) should work in this case. This is mainly because ObjectPtr is an intrusive pointer that directly leverages the object data structure.

To confirm, we can add ICHECK(sptr_to_self.get() == this);
``

…xecutable object for correct work of VirtualMachine on c++ side

vvchernov · 2022-01-31T14:26:04Z

Thanks @tqchen for your help, I've used your approch with GetObjectPtr. It looks like CI tests should pass soon. May be you recheck my last changes.

tqchen · 2022-01-31T23:04:42Z

Thanks @vvchernov !

Use ObjectPtr<Executable> instead of Executable* to solid saving of Executable object for correct work of VirtualMachine on c++ side Co-authored-by: Valery Chernov <valery.chernov@deelvin.com>

vvchernov requested review from ZihengJiang, areusch, junrushao, kazum, liangfu, masahi, tmoreau89, tqchen and vinx13 as code owners January 19, 2022 16:31

tqchen reviewed Jan 19, 2022

View reviewed changes

jroesch reviewed Jan 19, 2022

View reviewed changes

vvchernov force-pushed the vc/vm-fix branch from 5c6099a to c3eb6e2 Compare January 20, 2022 08:55

vvchernov mentioned this pull request Jan 20, 2022

[TVM EP] support of TVM Virtual Machine microsoft/onnxruntime#10341

Merged

tqchen reviewed Jan 22, 2022

View reviewed changes

tmoreau89 requested review from jroesch and tqchen January 27, 2022 01:32

vvchernov changed the title ~~WIP: [VirtualMachine] fix raw pointer using by VirtualMachine~~ [VirtualMachine] fix raw pointer using by VirtualMachine Jan 27, 2022

vvchernov force-pushed the vc/vm-fix branch from bf1b0bf to 2f33c8b Compare January 28, 2022 05:00

use ObjectPtr<Executable> instead of Executable* to solid saving of E…

65eb48f

…xecutable object for correct work of VirtualMachine on c++ side

vvchernov force-pushed the vc/vm-fix branch from 2f33c8b to 65eb48f Compare January 31, 2022 10:08

Valery Chernov added 2 commits January 31, 2022 13:28

fix clang-format lint

b091931

fix operator bool()

517f24d

tqchen approved these changes Jan 31, 2022

View reviewed changes

tqchen merged commit 36b48a5 into apache:main Jan 31, 2022

vvchernov deleted the vc/vm-fix branch October 4, 2022 07:04

Conversation

vvchernov commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tqchen Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

jroesch Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

jroesch Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

vvchernov Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvchernov commented Jan 21, 2022

Uh oh!

tqchen Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

vvchernov Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

tqchen Jan 22, 2022

Choose a reason for hiding this comment

Uh oh!

vvchernov Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqchen Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

tqchen commented Jan 22, 2022

Uh oh!

vvchernov commented Jan 26, 2022

Uh oh!

tmoreau89 commented Jan 27, 2022

Uh oh!

jwfromm commented Jan 27, 2022

Uh oh!

driazati commented Jan 27, 2022

Uh oh!

vvchernov commented Jan 28, 2022

Uh oh!

tmoreau89 commented Jan 28, 2022

Uh oh!

tmoreau89 commented Jan 28, 2022

Uh oh!

tqchen commented Jan 28, 2022

Uh oh!

vvchernov commented Jan 31, 2022

Uh oh!

tqchen commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vvchernov commented Jan 19, 2022 •

edited

Loading

vvchernov Jan 19, 2022 •

edited

Loading

vvchernov Jan 24, 2022 •

edited

Loading