On 7 Aug 2019, at 20:00, Mutz, Marc via Development <[email protected]<mailto:[email protected]>> 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 [email protected] https://lists.qt-project.org/listinfo/development _______________________________________________ Development mailing list [email protected]<mailto:[email protected]> https://lists.qt-project.org/listinfo/development
_______________________________________________ Development mailing list [email protected] https://lists.qt-project.org/listinfo/development
