Date: Thu, 14 Dec 2023 10:33:46 +0200
Thanks Marcin for noticing the race condition ... I rushed into the idea :(.
It seems that there is nothing we can do without support from the
shared_ptr implementation.
'reset_and_use_count' looks indeed as a viable option.
On Wed, Dec 13, 2023 at 10:57 PM Marcin Jaczewski <
marcinjaczewski86_at_[hidden]> wrote:
> śr., 13 gru 2023 o 19:31 Valentin Palade <vipalade_at_[hidden]> napisał(a):
> >
> > Agree with DoA.
> >
> > Will further rely on the following solution:
> > SharedMessageT collapse(SharedMessageT &_rsm, const ssize_t _expect = 1)
> > {
> > SharedMessageT::weak_type wt{_rsm};
> >
> > _rsm.reset();
> >
> > if(wt.use_count() == _expect){
> > return wt.lock();
> > }
> > return SharedMessageT{};
> > }
> >
>
> This logic have "data race". Consider this sequential code:
> ```
> auto _rsm1 = std::make_shared<T>();
> auto _rsm2 = _rsm1;
>
> SharedMessageT::weak_type wt1{_rsm1};
> SharedMessageT::weak_type wt2{_rsm2};
>
> _rsm1.reset();
> _rsm2.reset();
>
> if(wt1.use_count() == _expect) ok1 = true;
> if(wt2.use_count() == _expect) ok2 = true;
>
> if (ok1) wt1.lock();
> if (ok2) wt2.lock();
> ```
> As we can easily see, both weak pointers will lock.
> If this case have problems, even more real multithreading code
> will have.
>
> If `reset()` return user count after decrement (as one atomic
> operation) then we will have:
> ```
> SharedMessageT::weak_type wt{_rsm};
>
> if (_rsm.reset_and_use_count() == _expect)
> {
> return wt.lock();
> }
> ```
> Code like this should work if nobody try bump the number of shared
> pointers.
>
> > maintain a SharedMessageT instance on producer.
> >
> > and, on consumer use:
> > if (auto tmp_sm = collapse(sm)) {
> > p.set_value(std::move(tmp_sm));
> > }
> >
> > Thanks everyone for your valuable feedback.
> >
> >
> > On Tue, Dec 12, 2023 at 7:16 PM Marcin Jaczewski <
> marcinjaczewski86_at_[hidden]> wrote:
> >>
> >> wt., 12 gru 2023 o 18:01 Thiago Macieira <thiago_at_[hidden]>
> napisał(a):
> >> >
> >> > On Tuesday, 12 December 2023 08:56:21 -03 Valentin Palade wrote:
> >> > > The main requirements behind resurrect are:
> >> > >
> >> > > 1. lock free
> >> > > 2. preserving the control block that was allocated initially for
> the
> >> > > object.
> >> > >
> >> > > Because of the second idea, we cannot efficiently move away from
> the old
> >> > > pointer as we'll need to reallocate the control block.
> >> >
> >> > Your conclusion does not follow from the facts. Why do you think that
> moving
> >> > will cause it to reallocate?
> >> >
> >> > Yes, as discussed, depending on a race, you could end up freeing a
> block. I
> >> > wouldn't expect this to be a problem for an optimisation. It's just a
> best
> >> > effort.
> >> >
> >>
> >> I could argue a bit longer but Lénárd's email makes the proposal DoA.
> >> Existence of `weak_pointer` makes `resurrect` at best unreliable.
> >>
> >> > --
> >> > Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
> >> > Software Architect - Intel DCAI Cloud Engineering
> >> >
> >> >
> >> >
>
It seems that there is nothing we can do without support from the
shared_ptr implementation.
'reset_and_use_count' looks indeed as a viable option.
On Wed, Dec 13, 2023 at 10:57 PM Marcin Jaczewski <
marcinjaczewski86_at_[hidden]> wrote:
> śr., 13 gru 2023 o 19:31 Valentin Palade <vipalade_at_[hidden]> napisał(a):
> >
> > Agree with DoA.
> >
> > Will further rely on the following solution:
> > SharedMessageT collapse(SharedMessageT &_rsm, const ssize_t _expect = 1)
> > {
> > SharedMessageT::weak_type wt{_rsm};
> >
> > _rsm.reset();
> >
> > if(wt.use_count() == _expect){
> > return wt.lock();
> > }
> > return SharedMessageT{};
> > }
> >
>
> This logic have "data race". Consider this sequential code:
> ```
> auto _rsm1 = std::make_shared<T>();
> auto _rsm2 = _rsm1;
>
> SharedMessageT::weak_type wt1{_rsm1};
> SharedMessageT::weak_type wt2{_rsm2};
>
> _rsm1.reset();
> _rsm2.reset();
>
> if(wt1.use_count() == _expect) ok1 = true;
> if(wt2.use_count() == _expect) ok2 = true;
>
> if (ok1) wt1.lock();
> if (ok2) wt2.lock();
> ```
> As we can easily see, both weak pointers will lock.
> If this case have problems, even more real multithreading code
> will have.
>
> If `reset()` return user count after decrement (as one atomic
> operation) then we will have:
> ```
> SharedMessageT::weak_type wt{_rsm};
>
> if (_rsm.reset_and_use_count() == _expect)
> {
> return wt.lock();
> }
> ```
> Code like this should work if nobody try bump the number of shared
> pointers.
>
> > maintain a SharedMessageT instance on producer.
> >
> > and, on consumer use:
> > if (auto tmp_sm = collapse(sm)) {
> > p.set_value(std::move(tmp_sm));
> > }
> >
> > Thanks everyone for your valuable feedback.
> >
> >
> > On Tue, Dec 12, 2023 at 7:16 PM Marcin Jaczewski <
> marcinjaczewski86_at_[hidden]> wrote:
> >>
> >> wt., 12 gru 2023 o 18:01 Thiago Macieira <thiago_at_[hidden]>
> napisał(a):
> >> >
> >> > On Tuesday, 12 December 2023 08:56:21 -03 Valentin Palade wrote:
> >> > > The main requirements behind resurrect are:
> >> > >
> >> > > 1. lock free
> >> > > 2. preserving the control block that was allocated initially for
> the
> >> > > object.
> >> > >
> >> > > Because of the second idea, we cannot efficiently move away from
> the old
> >> > > pointer as we'll need to reallocate the control block.
> >> >
> >> > Your conclusion does not follow from the facts. Why do you think that
> moving
> >> > will cause it to reallocate?
> >> >
> >> > Yes, as discussed, depending on a race, you could end up freeing a
> block. I
> >> > wouldn't expect this to be a problem for an optimisation. It's just a
> best
> >> > effort.
> >> >
> >>
> >> I could argue a bit longer but Lénárd's email makes the proposal DoA.
> >> Existence of `weak_pointer` makes `resurrect` at best unreliable.
> >>
> >> > --
> >> > Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
> >> > Software Architect - Intel DCAI Cloud Engineering
> >> >
> >> >
> >> >
>
Received on 2023-12-14 08:33:58