[Development] atomic reference counting implementation

[Lars Knoll]

> On 8 Aug 2019, at 10:55, Mutz, Marc via Development <development at qt-project.org> wrote:
> 
> 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?

Yes, sorry.
> 
>> 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.

QSharedData btw doesn’t define at all how the refcounting is done. Neither does btw, QSDP or QESPD. The documentation only talks about atomic reference counting, not that ref/deref is ordered. 

Changing how we increase/decrease the refcount in those classes is something we can do. Of course, you can always construct an example where this change could break something, but you can do the same for most bug fixes we do in Qt.
> 
> 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.

Why? QSharedData doesn’t have any implementation. It’s just a wrapper around an atomic int.
> 
> 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.

QISP is nice as it’s a bit more generic than QSDP, but I would argue that we could refactor QSDP to allow the same kind of flexibility in a SC fashion. So do we really need a new class here?
> 
> - (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.

Even if we accept not to change QAtomic::ref/deref() (something I still double would break in more than one or two places in all the existing code out there), I do not see any problems with changing this for QSDP AND QESDP, especially for Qt 6.
> 
> 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).

Even if we don’t change QAtomic, there seems to be also little need to introduce whole new classes because QSD, QSDP and QESDP do *not* specify that refcounting is ordered.

Cheers,
Lars