Optimize matrix_cl constructors - #1421

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.99)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.01)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 0.98)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.01)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.99)
(performance.compilation, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.03)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.03)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 0.99)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 0.99)
Result: 1.00150312367
Commit hash: cbe1033

@t4c1, want to video chat sometime today or tomorrow? @SteveBronder, you free too? I'd like to discuss:

1. design. Have you talk through why it should be done this way, if you've evaluated the other ways to accomplish this, if there's some way to be a little more safe about this.
2. required things to do on the PR regardless of design. I think we've identified that this hasn't met the requirements of doxygen documentation and the PR description should be updated. I haven't even had a chance to see if there are tests that match, so we might have to do that.

Thanks for starting a new thread. I'll respond there for discussion about "side effects."

Also... mind fixing the code conflict? We can't move on this PR until that's taken care of too.

That conflict is due to a refactor PR #1407 being reverted because of a bug. After that is fixed and refactor gets back into develop this conflict should go away.

We can do a chat tommorow. I have time between 8h and 15h CET.

Ah. I don't think I'm going to be able to make those times tomorrow. (I'm in eastern time zone... if I interpreted the times correctly, that's 2am-9am). Usually, I'd be able to make 14h CET, but tomorrow I have an early meeting.

Friday?

I could do Friday 14-15 CET

Me too.

Friday 14h CET it is!

Awesome. Talk to you tomorrow! I'm sending conference info via a Discourse pm.

Ack sorry, I don't understand the concept of time zones and thought this was at 9 AM EST.

Was going to ask why we don't just use a shared pointer here instead of the callback. Since then we can keep track of reference counts and it automatically deletes when there are zero references. Little bulky but very clean

I just got off a video call with @t4c1. This is neat! Some things that happened:

1. @t4c1 walked me through the design. This one makes sense. He was able to walk me through the common patterns of managing memory and explained how they actually don't quite work out. Specifically, he walked me through the logic of how the common pattern of having a member variable storing the location of the allocated memory isn't ideal. There are two constraints driving the existing design:

   1. this is asynchronous code. In order to handle things with different callbacks or hold onto the reference and delete in the destructor, we'd have to be threadsafe and wait for certain things to happen, which are often out of our control.
   2. early recovery of allocated memory. Using the callback allows the memory to be released back into the system when the function is called. Under most normal conditions, this will happen within the lifetime of the matrix_cl object, so it makes sense not to have it when destructed.

   That leads to the current design making sense, even though on first glance it looks like a leak. The next point addresses that.

2. We discussed some of the required documentation. Things like initialize_buffer() assume ownership of memory under certain conditions and it's not clear what the requirements are for the function. @t4c1 is going to update some of the documentation to make things clearer.

3. @t4c1 found a spot in the constructor that needed to be fixed; he has the details and will fix it.

Overall, the state of the code is nearly there. What is required prior to merging is an update on the documentation so it's clear why this deviates from a standard pattern. That should also include the required documentation on functions mentioning their requirements on memory persistence so it's clear to the next reader what's happening.

Thanks, @t4c1!

@SteveBronder If we used shared pointer (or any other smart pointer) an store it in the matrix_cl object we could not release the memory until the matrix_cl object goes out of scope. This way we can release the memory as soon it is not needed anymore (which is often much sooner than the matrix goes out of scope).

idt you even need to store it though since the shared_ptr is not deleted until it's reference count goes to zero no? You could just make one, pass it into initialize_buffer then once the last reference goes away all is well

That sounds right? I'll mess with this later to see if it works

Reference count for the shared ponter is the same thing as the number of shared pointer objects that hold the same pointer. So if we create shared pointer in constructor and don't store it as a member variable that shared ponter will go out of scope when the constructor returns and the memory will be deleted. This is not what we want.

@t4c1, this has conflicts that has to be resolved. (Sorry -- I didn't realize you added the doc; adding commits alone does not notify reviewers.)

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.99)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 0.98)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 1.0)
(performance.compilation, 1.02)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 1.0)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.01)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 1.0)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 0.97)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.01)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 0.97)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.02)
Result: 0.99683340255
Commit hash: caab623

@syclik This is now ready for review.

@SteveBronder, I "dismissed" your review on the PR. @t4c1 went through and addressed all the comments and I checked it over. There was one outstanding question that was in your court, but since it went unanswered, I think it's fine.

Sorry, I stopped responding because I'm uncertain whether this is good or not. If your both comfortable with it then I'm fine with abstaining

I've also been pretty busy at work and a lot of the stuff in this PR was unclear and I didn't have time to meaningfully look at it

It’s all good.

The important things are:

1. It’s documented we’ll enough for someone to pick up in the future.
2. It’s passing tests.
3. It had been reviewed carefully.

As with most implementations, we’re not looking for the perfect solution. Things change and hopefully we know enough to fix it if it needs to change in the future.