Beats: Use std::make_shared/shared_from_this() #4336

uklotzde · 2021-09-29T11:02:24Z

...and fix various inconsistencies and other flaws along the way.

Do not return a nullptr to avoid unintended crashes!

tcoyvwac · 2021-09-29T13:50:37Z

Was about to PR the same thing. 😄

uklotzde · 2021-09-29T14:21:50Z

@tcoyvwac Do you know of a better solution than the artificial, protected type tag for making the public constructors inaccessible? The other changes should be non-controversial.

uklotzde · 2021-09-29T14:24:48Z

The design is still unfinished. An immutable, sharable wrapper should wrap plain, mutable C++ classes that do not need to care about the sharing that happens in the outer context. I am not going to work on this clean separation of functional from infrastructure code.

uklotzde · 2021-09-29T14:30:57Z

In the long term we need to think about applying the same principles to other areas like Cues and the bloated Track class with their error prone internal locking. There we are facing additional challenges like how to embed the design into connected QObjects in the UI layer. The current design is a big mess.

daschuer · 2021-09-29T20:17:39Z

Did you have a look how Qt implements it implicit sharing and private implementation pattern?

I think this would work here also fine and release the using code form all the pointer and stuff.

The internal container holding the immutable beats can be implemented as a cpp file only class. The outside copy-able wrapper will point to the internal container and can be handled by value in the code.

I have in mind to make just like QByteArray, In your case it is an array of beats and their metadata.
We need to skip the implementation of copy-on-write and have our immutable object ready. This comes with the same thread safety Qt has, when sending it via signals and slot connections.

In Qt 6 they call it DataPointer d.

We don't even need to hide the the internal class, because we have no public API.

What do you think?

uklotzde · 2021-09-29T21:00:00Z

I am not interested in rewriting these classes besides the obvious improvements and safety fixes provided by this PR. Please review the PR or I will close it.

uklotzde · 2021-09-29T21:04:13Z

If you have ideas that extend the scope of this PR then please provide a follow-up PR for review.

tcoyvwac · 2021-09-30T07:58:50Z

@tcoyvwac Do you know of a better solution than the artificial, protected type tag for making the public constructors inaccessible? The other changes should be non-controversial.

No, currently, I can not think of a way.

The intent though of your PR is very clear and also agree with you that refactoring classes (via an API analysis approach) right now is out of the scope of this PR.

This PR is just about locking down the dynamic parts of the projects around smart pointers (using modern methods). The faster the "safety" PRs are merged, then the more flexibility there is (or "will be") in redesigning some current API structures (safely).

daschuer · 2021-09-30T21:22:57Z

src/track/beatgrid.cpp

@@ -98,7 +98,8 @@ BeatsPointer BeatGrid::makeBeatGrid(
    // Calculate beat length as sample offsets
    const audio::FrameDiff_t beatLengthFrames = 60.0 * sampleRate / bpm.value();

-    return BeatsPointer(new BeatGrid(sampleRate, subVersion, grid, beatLengthFrames));
+    return std::make_shared<BeatGrid>(
+            MakeSharedTag{}, sampleRate, subVersion, grid, beatLengthFrames);


Why do you use here the explicit version, where above you use only the braced tantalizer?

Because std::make_shared() is not able to deduce the type and compilation fails. Try it.

I think we should be explicit everywhere.

daschuer · 2021-09-30T21:27:12Z

src/track/beatgrid.cpp

+BeatGrid::BeatGrid(
+        MakeSharedTag,
+        const BeatGrid& other)
+        : BeatGrid({}, other, other.m_grid, other.m_beatLengthFrames) {


Why you don't pass the MakeSharedTag, but create a new one?

Because type tags are not supposed to carry any information and should be eliminated during compilation. Passing the instance is not desired in this special case. Just a hint for the compiler to ignore the unnamed parameter.

daschuer

Thank you for the PR, I have left some comments for better readability.

daschuer · 2021-09-30T21:38:55Z

src/track/beats.h

  public:
    virtual ~Beats() = default;

+    BeatsPointer clone() const {


The term clone() is misleading here, because the Beats object is not cloned in terms of making an identical copy.
You just copy a shared pointer form a hidden weak pointer. In Qt it is called toStrongRef()
In our case maybe toBeatsPointer() or getBeatsPointer() or such.

Cloning an immutable object is trivial be sharing its reference.

Conceptually this is still a clone operation.

I was confused when reading it the first time. You can either add documentation that this does not really clone, or change the name. I would prefer the later.

daschuer · 2021-09-30T21:47:00Z

src/track/beats.h

+    /// Type tag for making public constructors of derived classes inaccessible.
+    ///
+    /// The constructors must be public for using std::make_shared().
+    struct MakeSharedTag {};


This is not only used by makeShared. It is more like a "CTorProtect" or "ProtectedTag"

It is only needed for make_shared(). Otherwise the constructors could be hidden by making them private.

I disagree. Redundantly repeating the type name of the type tag class where it is not needed is useless. It must only specified explicitly when using std::make_shared(), the only purpose it was introduced for.

We hit often such discussion in our reviews. I think this is sourced by our different roles.
For me as a reviewer it is important to understand the code at once without looking into other references.
You as the author are more interested to avoid redundant statements.
Please keep in mind that the human reader is not a compiler which reads thousands of lines is a second, fetched form many included files. To make the code readable I prefer redundant statements more than explaining comments, because the compiler can check them.

A plain {} as a parameter can be everything, the reader needs to look up other references to understand it. This is the same for bool function parameter by the way.

Whenever the function signature changes, a {} will still compile even though it might be wrong.

The plan was to use {} everywhere but std::make_shared() fails to deduce the type.

I don't see a reason to re-introduce the redundant type name in private forwards.

daschuer · 2021-09-30T22:00:39Z

src/track/beatgrid.cpp

@@ -98,7 +98,8 @@ BeatsPointer BeatGrid::makeBeatGrid(
    // Calculate beat length as sample offsets
    const audio::FrameDiff_t beatLengthFrames = 60.0 * sampleRate / bpm.value();

-    return BeatsPointer(new BeatGrid(sampleRate, subVersion, grid, beatLengthFrames));
+    return std::make_shared<BeatGrid>(
+            MakeSharedTag{}, sampleRate, subVersion, grid, beatLengthFrames);


I think we should be explicit everywhere.

daschuer

LGTM, thank you.

daschuer · 2021-10-01T07:55:05Z

I wanted to know it exactly and unfortunately your assumption is wrong that the compiler is able to optimize MakeSharedTag{} away.

I have compiled the original version and a version where I have just deleted every MakeSharedTag{} coinsurance, than I have called:
objdump -S build/Debug/CMakeFiles/mixxx-lib.dir/src/track/beatgrid.cpp.o > beatgid.cpp

Here the interesting snippet.
Original:

    return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, m_beatLengthFrames);
}
    1575:	48 8b 44 24 38       	mov    0x38(%rsp),%rax
    157a:	64 48 33 04 25 28 00 	xor    %fs:0x28,%rax
    1581:	00 00 
    1583:	75 6f                	jne    15f4 <_ZNK5mixxx8BeatGrid9translateEd+0x2a4>
    1585:	48 83 c4 48          	add    $0x48,%rsp
    1589:	4c 89 e0             	mov    %r12,%rax
    158c:	5b                   	pop    %rbx
    158d:	5d                   	pop    %rbp
    158e:	41 5c                	pop    %r12
    1590:	41 5d                	pop    %r13
    1592:	41 5e                	pop    %r14
    1594:	41 5f                	pop    %r15
    1596:	c3                   	retq   
    1597:	66 0f 1f 84 00 00 00 	nopw   0x0(%rax,%rax,1)
    159e:	00 00 
  return p != NULL ? *p : *reinterpret_cast<const ::mixxx::track::io::Bpm*>(

    return std::make_shared<BeatGrid>(*this, grid, m_beatLengthFrames);
}
    1430:	48 8b 44 24 38       	mov    0x38(%rsp),%rax
    1435:	64 48 33 04 25 28 00 	xor    %fs:0x28,%rax
    143c:	00 00 
    143e:	75 54                	jne    1494 <_ZNK5mixxx8BeatGrid9translateEd+0x234>
    1440:	48 83 c4 40          	add    $0x40,%rsp
    1444:	4c 89 e0             	mov    %r12,%rax
    1447:	5b                   	pop    %rbx
    1448:	5d                   	pop    %rbp
    1449:	41 5c                	pop    %r12
    144b:	41 5d                	pop    %r13
    144d:	41 5e                	pop    %r14
    144f:	c3                   	retq   
  return p != NULL ? *p : *reinterpret_cast<const ::mixxx::track::io::Bpm*>(

nopw 0x0(%rax,%rax,1) is the 0 initializing "constructor" of the MakeSharedTag which can not be optimized away.

the over all size of the .text segment changes form 0x1cd9 to 0x1ad1 this means 520 bytes of code saved.

I have also checked if it makes a difference to the forwarded constructor if the tag is copied or if a new one is created.
Result: Both is exactly the same

daschuer · 2021-10-01T08:08:02Z

Conclusion: An empty struct is not optimized away if it is part of the public interface. I think that is different when it is used in header only templates.

Here we protect the code against low risk of missused with a penalty at runtime. I don't think this will pay off as a general pattern.

What is the bad think that may happen if one uses the constructor directly?

uklotzde · 2021-10-01T08:16:27Z

...which doesn't invalidate any of my statements besides the wrong conclusion at runtime that doesn't matter. The type tag is only needed at compile time and should be treated as void at runtime.

If you are keen on zero-cost abstractions I recommend switching to Rust instead of bothering with C++ ;)

uklotzde · 2021-10-01T08:18:25Z

https://en.cppreference.com/w/cpp/memory/enable_shared_from_this/shared_from_this

Otherwise the behavior is undefined (until C++17)std::bad_weak_ptr is thrown (by the shared_ptr constructor from a default-constructed weak_this) (since C++17).

Trading in a non-zero-sized, empty struct to catch errors at compile time for an exception at runtime would be a really bad deal.

uklotzde added 5 commits September 29, 2021 12:10

Beats: Add missing const qualifiers

6cb1da2

Beats: Delete obsolete comments

1bbee5f

Beats: Use std::enable_shared_from_this

df574b1

Beats: Use std::make_shared

beaf525

Beats: Unify behavior of scale()/translate() operations

27be226

Do not return a nullptr to avoid unintended crashes!

uklotzde added the code quality label Sep 29, 2021

uklotzde requested review from daschuer and Holzhaus September 29, 2021 11:02

daschuer reviewed Sep 30, 2021

View reviewed changes

uklotzde added 2 commits October 1, 2021 01:55

Beats: Add redundant type name

91b5d8c

Beats: Rename clone() function

90d362b

daschuer approved these changes Oct 1, 2021

View reviewed changes

daschuer merged commit 8b1ed1b into mixxxdj:main Oct 1, 2021

uklotzde deleted the beats branch October 1, 2021 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beats: Use std::make_shared/shared_from_this() #4336

Beats: Use std::make_shared/shared_from_this() #4336

uklotzde commented Sep 29, 2021

tcoyvwac commented Sep 29, 2021

uklotzde commented Sep 29, 2021

uklotzde commented Sep 29, 2021 •

edited

Loading

uklotzde commented Sep 29, 2021

daschuer commented Sep 29, 2021

uklotzde commented Sep 29, 2021

uklotzde commented Sep 29, 2021

tcoyvwac commented Sep 30, 2021 •

edited

Loading

daschuer Sep 30, 2021

uklotzde Sep 30, 2021

daschuer Sep 30, 2021

daschuer Sep 30, 2021

uklotzde Sep 30, 2021

daschuer left a comment

daschuer Sep 30, 2021

uklotzde Sep 30, 2021

uklotzde Sep 30, 2021

daschuer Sep 30, 2021

daschuer Sep 30, 2021

uklotzde Sep 30, 2021

uklotzde Sep 30, 2021

daschuer Sep 30, 2021

uklotzde Sep 30, 2021

daschuer Sep 30, 2021

daschuer left a comment

daschuer commented Oct 1, 2021

daschuer commented Oct 1, 2021

uklotzde commented Oct 1, 2021

uklotzde commented Oct 1, 2021 •

edited

Loading

Beats: Use std::make_shared/shared_from_this() #4336

Beats: Use std::make_shared/shared_from_this() #4336

Conversation

uklotzde commented Sep 29, 2021

tcoyvwac commented Sep 29, 2021

uklotzde commented Sep 29, 2021

uklotzde commented Sep 29, 2021 • edited Loading

uklotzde commented Sep 29, 2021

daschuer commented Sep 29, 2021

uklotzde commented Sep 29, 2021

uklotzde commented Sep 29, 2021

tcoyvwac commented Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Oct 1, 2021

daschuer commented Oct 1, 2021

uklotzde commented Oct 1, 2021

uklotzde commented Oct 1, 2021 • edited Loading

uklotzde commented Sep 29, 2021 •

edited

Loading

tcoyvwac commented Sep 30, 2021 •

edited

Loading

uklotzde commented Oct 1, 2021 •

edited

Loading