on_scheduler_exit unused

[bbbales2]

Description

Is there a reason on_scheduler_exit is unused? (https://github.com/stan-dev/math/blob/develop/stan/math/rev/core/init_chainablestack.hpp#L50)

Should we put it in the destructor or not use it?

Current Version:

v3.1.0

[bbbales2]

Also why is this second if here: https://github.com/stan-dev/math/blob/develop/stan/math/rev/core/autodiffstackstorage.hpp#L128

[bob-carpenter]

From the doc:

How is thread safety ensured for creating the singleton? It looks like creating instance_ would cause a race condition as the doc says:

The singleton instance_ is a global static pointer

Is there a reason we can't just use this?

static const AutodiffStackStorage* singleton() {
  static  const AutodiffStackStorage instance;
  return &instance;
}

I thought it was guaranteed to be thread safe and only called once. But I feel like we had this discussion already.

If the init() static class function's needed for some reason, then it might be easier to understand written as:

static bool init() {
  static STAN_THREADS_DEF bool is_initialized = false;
  if (is_initialized && instance_) { return false; }
  is_initialized = true;
  instance_ = new AutodiffStackStorage();
  return true;
};

[wds15]

@bbbales2 on_scheduler_exit is unused in the Stan programs as we use them, because thread cease to exist on program termination. In other applications there could be the possibility that the C++ application creates threads, uses them with the TBB and then terminates the thread before exiting the program. In this case the on_scheduler_exit is used.

@bob-carpenter What you propose is a Mayer singleton approach. We used that in the past, but it is slow as it gives you a ~20% performance hit when this thing is thread local. The admittedly odd gymnastic in init() is needed to avoid a static init order catastrophe, which requires that we do not access any globals where we cannot be sure of initialization status at the time-point when the program flows over it.

[bob-carpenter]

On Feb 26, 2020, at 12:32 AM, wds15 ***@***.***> wrote: @bbbales2 on_scheduler_exit is unused in the Stan programs as we use them, because thread cease to exist on program termination. In other applications there could be the possibility that the C++ application creates threads, uses them with the TBB and then terminates the thread before exiting the program. In this case the on_scheduler_exit is used.

Should we have the exit called before exiting a main() so we don't dangle resources (even if they will go away).

@bob-carpenter What you propose is a Mayer singleton approach. We used that in the past, but it is slow as it gives you a ~20% performance hit when this thing is thread local. The admittedly odd gymnastic in init() is needed to avoid a static init order catastrophe, which requires that we do not access any globals where we cannot be sure of initialization status at the time-point when the program flows over it.

Thanks for the reminder. I couldn't remember if the pattern I was suggesting was broken or slow, but I do remember it from before.

[wds15]

There are no resources left behind. The unique ptr will manage the stack resource. The main point about the exit function is to deregister the exiting thread from the unordered map which is obsolete as it is destructed.

[bbbales2]

on_scheduler_exit is unused in the Stan programs as we use them, because thread cease to exist on program termination'

Should we have the exit called before exiting a main() so we don't dangle resources (even if they will go away).

By adding an on_scheduler_exit function to the destructor do we solve this?

[bbbales2]

Oh I see. All it is doing is deleting something from a map.

[bbbales2]

Also regarding the init(), is this attached to the rstan segfault at all? That code is using an older init() which only has one if in it.

[wds15]

By adding an on_scheduler_exit function to the destructor do we solve this?

I don't think we need to care about this. All of this is managed by the Intel TBB.

Oh I see. All it is doing is deleting something from a map.

Yup. Resources are not managed by the object directly.

Also regarding the init(), is this attached to the rstan segfault at all? That code is using an older init() which only has one if in it.

I don't think so as rstan is with 2.19 where we did not yet have the new TLS stuff in 2.19.

Can we close this issue?

[bbbales2]

Nono, I was talking about the rstan issue we're talking to BGoodrich about now. That is develop RStan and some new StanHeaders.

Can we close this issue?

Yeah but this code needs doc'd.

[bbbales2]

Actually wait I think we need to leave this open? I'm confused. I don't understand the execution context of this. We should probably just talk through it at the next Math meeting.

If it makes sense to ever destruct these objects and build them again, the destruction better be threadsafe, and it sure seems like from on_scheduler_exit there's some locks and whatnot in the deallocation of the map.

[wds15]

Should we change the issue? I mean the method is used for sure in some contexts. Talking about this is probably easiest.

EDIT: Did you read

https://software.intel.com/en-us/node/506314

and

https://software.intel.com/en-us/node/506315

?

That's very helpful.

The basic idea is that threads entering and leaving the TBB threadpool fire off a visitor type of event which we use to initialize the autodiff tape and tear it down. This way, the C++ user always gets a ready to use AD tape for a given TBB thread.

[bbbales2]

Yeah rename to whatever. @syclik hopefully we can remember to talk about this in next Math meeting.