[Development] atomic reference counting implementation

[Mutz, Marc]

On 2019-08-08 09:36, Lars Knoll wrote:
>> On 7 Aug 2019, at 20:00, Mutz, Marc via Development
[...]
> Changing ref() from seq_cst to relaxed and documenting that should be
> ok for Qt 6, as it’ll still to the right thing for its indented use
> case. At least it is very much preferable over duplicating all our
> classes.
> 
>> I think I'm on record saying such impl details as ref-counting for
>> Qt implicitly shared classes should not be public API. This is a
>> perfect example of why I believe that to be fundamentally true.
> 
>  QSharedPointer

QS_Data_P?

> helped our users implement their own ref-counted
> classes. Having such a class is a good thing. And if some of its
> semantics are stricter than they should have been, let’s fix that.

Careful not to mix things here: What we need to change is not QSDP or 
QESDP. At least not for this particular problem. Q(E)SDP's problems lie 
elsewhere and it's just a lucky coincidence that QISP allows to fix 
both: the a) Q(E)SDP problems and b) taking Qt ref-counting private to 
allow the proposed optimisation.

What needs to change for (b) is ref() (and deref()), and I think 
QSharedData, but Peppe already doubted that and he may have a point. I'm 
saying that we can have both: old ref() with the same semantics and 
something Qt-private to use in our own ref-counting. QSharedData isn't 
rocket science, and it's easy to do the change while porting to QISP.

That leaves the question of whether "QSharedDataV2" should be public or 
private API. I'm not sure. I'd very much prefer to have this as a 
private implementation detail of Qt, and keep QSharedData as the public 
class.

Let's look at the possible users:

- applications programmers can continue to use QSD and "suffer" from the 
performance drag of ordered ref(). They can also use QISP + std::atomic 
as ref-count and implement their own qIntrusiveRef/Deref to match what 
Herb and Hans recommend.

- (Qt-using) library programmers can use qshareddata_p.h and selectively 
opt into the "fast" ref-counting. For most Qt-centric libraries, this 
will probably not bring any new dependencies.

I can also spell QSharedDataV2 "QIntrusivelyRefCounted" and make that 
public. I'm confident that with QISP and QSDv2, we won't need to have 
yet another pimpl class in a few years. I wasn't so confident with 
QtPrivate::SharedDataPointer.

- Qt libraries should all be ported to QISP and QSDv2.

> This goes back to what the classes/methods intended and documented use
> is. ref() is there to increase a refcount

But that's the whole point Olivier was making two years ago: ref() is 
_documented_ to be a memory barrier:

https://doc.qt.io/qt-5/qatomicinteger.html#ref:
> This function uses ordered memory ordering semantics,
> which ensures that memory access before and after the
> atomic operation (in program order) may not be re-ordered.

> , and for that relaxed
> semantics are enough. If you need seq_cst, we have a method that
> explicitly does that in QAtomic as well.

Experience shows that even "obviously correct" changes in documented 
behaviour can backfire (QString(QByteArray) comes to mind). Users may 
have used ref() as a synchronisation point, because it was documented to 
be. I find it hard to believe that users would be able to find such uses 
in their code bases even if they knew ref() has changed.

This is the worst of silent breakages: no known (easy) pattern to search 
for, so high chance of being missed even when audited for. While 
probability of the API being used in this particular way is low, the 
severity is fatal: UB. It's like having a mutex which in some random 
cases doesn't lock.

If you want to change ref(), call it something else and deprecate or 
remove ref() to make its use a compile error.  But there's absolutely 
zero need to touch QAtomic or QSharedData: they can live on (deprecated 
or not, in QtCore or Qt5Support, I don't care), untouched, while the 
world migrates to the improved successors (std::atomic and QISP).

Thanks,
Marc