fixstars / ion-kit

Modernized graph-based data processing framework
MIT License
7 stars 6 forks source link

Auto disposer is not called in graph API #255

Closed xinyuli1204 closed 3 months ago

xinyuli1204 commented 4 months ago

When I use builder.run(), the disposer works fine with simple_graph_jit

~Builder is called
~Builder::Impl is called

However, the graph disposer didn't call ~Impl if I use graph api

      Builder b;
      b.with_bb_module("ion-bb-test");
      b.set_target(Halide::get_host_target());

      Graph g = b.add_graph("graph0");
      Node n;
      n = g.add("test_producer").set_param(Param("v", 41));
      n = g.add("test_consumer")(n["output"], &min0, &extent0, &min1, &extent1, &v);

      ion::Buffer<int32_t> r = ion::Buffer<int32_t>::make_scalar();
      n["output"].bind(r);

      for (int i=0; i<5; ++i) {
          std::cout << i << "'th loop" << std::endl;
          g.run();
      }
~Graph is called
~Builder is called

I expect the correct result to be something like this:

~Graph is called
~Graph::Impl is called
~Builder is called
~Builder::Impl is called

The possible reason may be ownership cycles and looks like a circular dependency chain between graph and builder, causing the shared_ptr to never get cleaned up. If I change Graph g = b.add_graph("graph0"); to Graph g = Graph(b,"graph0"); It works fine with calling the ~Impl.

xinyuli1204 commented 3 months ago

@iitaku

Can I use reference or raw pointer instead weak_ptr?

1. Weak pointer

I am not sure if I am doing it right. using weak pointer, I am trying to do something like this...


struct Graph::Impl {
    std::weak_ptr<Builder>  builder;
   .........

    Impl(std::weak_ptr<Builder> b, const std::string& n)
        : id(sole::uuid4().str()), builder(b), name(n), jit_ctx(new Halide::JITUserContext), jit_ctx_ptr(jit_ctx.get())
    {
    }

Details are in this commit And also need to change add_graph api in builder.cc.

previously it is

Graph Builder::add_graph(const std::string& name) { Graph g(*this, name); impl_->graphs.push_back(g); return g; }

and now I need to change something like

Graph Builder::add_graph(const std::string& name) {
   auto shared_ptr = shared_from_this();
    Graph g(shared_ptr , name);
    impl_->graphs.push_back(g);
    return g;
}

However shared_from_this throw out bad weak_ptr_errorsince shared_from_this() is called on an Builder object that is not managed by a std::shared_ptr.

Sorry I don't know if I can use weak pointer at this time..

2. Reference

Please see here https://github.com/fixstars/ion-kit/pull/256/files Currently it works fine and doesn't change code too much.

Fixstars-iizuka commented 3 months ago

@xinyuli1204 I see. You can use Reference at this moment.

xinyuli1204 commented 3 months ago

done by https://github.com/fixstars/ion-kit/pull/256