[Development] atomic reference counting implementation

[Lars Knoll]

On 7 Aug 2019, at 20:00, Mutz, Marc via Development <development at qt-project.org<mailto:development at qt-project.org>> wrote:

Hi Philippe,

This was discussed in https://codereview.qt-project.org/c/qt/qtbase/+/66118. See, in particular, Olivier's comment.

TL;DR: ref() is documented to be ordered, so cannot be changed.

Consequently, I didn't merge it even with Thiago's +2.

To fix, we'd need to port all of the Qt classes away from the public classes (QSharedData, QAtomicInt) and implement ref-counting completely from scratch (well, copy QSharedData → QSharedDataV2, QAtomicInt → QAtomicIntV2, and do the change in V2). That, or we introduce silent breakages into user code by applying 66118, something our Chief Whip has just announced publicly should not happen:

https://blog.qt.io/blog/2019/08/07/technical-vision-qt-6/
If we must break compatibility, a compile-time error is preferable
over a silent breakage at runtime (as those are much harder to detect).

Notice ‘preferable’. Not ‘must be’. This depends fully on the amount of breakage it can introduce. No behavioural changes at all would not allow us to do any bug fixes ;-)

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 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.

This goes back to what the classes/methods intended and documented use is. ref() is there to increase a refcount, and for that relaxed semantics are enough. If you need seq_cst, we have a method that explicitly does that in QAtomic as well.

Cheers,
Lars


Thanks,
Marc

On 2019-08-07 19:36, Philippe wrote:
I recently found that in Qt,  reference counting to guard a resource, is using
ref() / deref()
But ref() is using memory_order_seq_cst
while memory_order_relaxed, should be sufficient
What is important is to guarantee that destruction is not subject to a
race condition, to prevent double
destruction. Hence deref() with memory_order_seq_cst is enough to guarantee
that.
It does not matter how much the counter increase, but what is important
is to control how it is decreased. Hence deref(with memory_order_seq_cst)
is just enough.
I have verified the implementaiton of reference counting for shared_ptr
in clang, and it does what I describe above
(it even just use memory_order_acq_rel to decrement, and not
memory_order_seq_cst)
https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L3344
Is there a reason why Qt is not optimized in the same way? (since ref() is
used a lot, and atomic operations are a bit expensive).
Is there a requirement at some stage that the reference counter must be
ordered for increments?
Philippe
_______________________________________________
Development mailing list
Development at qt-project.org
https://lists.qt-project.org/listinfo/development
_______________________________________________
Development mailing list
Development at qt-project.org<mailto:Development at qt-project.org>
https://lists.qt-project.org/listinfo/development

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20190808/37d89df1/attachment.html>