gnuradio / newsched

The GNU Radio 4.0 Runtime Proof Of Concept
GNU General Public License v3.0
22 stars 16 forks source link

Work IO class to further simplify work() method #255

Closed mormj closed 2 years ago

mormj commented 2 years ago

Up until now, the work() method took in 2 vectors of pointers to the input/output structs. This groups all that under a work_io class that has a few benefits:

1) The io structure can be solidified when a block is given buffers - see populate_work_io in block.cc. This creates less churn and bookkeeping in the graph executor code iterating through ports and matching them up with the work_io vectors 2) Buffers can be referenced by port name in the work function - though a slight bit of overhead, when a block has a lot of ports, this can reduce problems by mis-indexing ports 3) A good place to put convenience functions like consume_each

mormj commented 2 years ago

The end result is that a work function now looks like:

work_return_code_t null_source_cpu::work(work_io& wio)
{
    auto itemsize = wio.output(0)->buffer->item_size();
    void* optr;
    for (size_t n = 0; n < wio.nout(); n++) {
        optr = wio.output(n)->items<void>();
        auto noutput_items = wio.output(n)->n_items;
        memset(optr, 0, noutput_items * itemsize);
        wio.output(n)->n_produced = noutput_items;
    }

    return work_return_code_t::WORK_OK;
}
mormj commented 2 years ago

@willcode - interested in your feedback on this change before I fully propagate it. it is a major API change, but functionally not that different. It does simplify a few things throughout the code, though

mormj commented 2 years ago

Removed the sptrs from the work_input and work_output vectors stored in work_io Wrapped the vectors/maps in a wrapper class so they can be referenced by index or name

Still confused a bit about how this will work with pybind11 for python blocks - when the scheduler calls work() that passes work_io to the python implementation, pybind11 needs to have a reference to the object, and I'm not sure yet how to do this. sptrs make this easy

mormj commented 2 years ago

Here is the part that is tripping me up with pybind:

work_return_code_t
python_sync_block::work(work_io& wio)
{
    py::gil_scoped_acquire acquire;
    py::object ret = d_py_handle.attr("work")(wio);
    return ret.cast<work_return_code_t>();
}

Normally pybind does a copy of the object, but with the copy constructor deleted, it doesn't by default pass wio by reference apparently.

mormj commented 2 years ago

I think I need to pass around a unique_ptr to work_io to get pybind to work. It appears that even when passed as reference:

py::object ret = d_py_handle.attr("work")(&wio);

the pybind11 code is creating a new instance of the work_io type

auto inst = reinterpret_steal<object>(make_new_instance(tinfo->type));

Which means in the python work function, changes to the fields do not make it back into the work_io object that was passed in

Edit: I take this back - still working through the details of how objects are passed by reference in pybind11, but I think it is workable Edit2: I think I have the reference policies figured out now for pybind

mormj commented 2 years ago

@marcusmueller @ThomasHabets - thank you so much for reviewing here and correcting some serious flaws. I think it is safe now to propagate to the rest of the blocks, even if the underlying implementation changes slightly, the api should be consistent (work(work_io&)).

After this I plan to go through and clean up some of the other shared_ptr abuse

mormj commented 2 years ago

Going ahead and squash/merging this. Thanks again for all the review here. Any other cleanup can go in another PR, and especially any further removal of sptr abuses will have to be done separately.

Would still like guidance/insight on to what level that cleanup will be worth it.