CoverArtCache refactoring + Fix scrolling lag after updating Mixxx #12009

daschuer · 2023-09-22T08:54:17Z

This is a prerequisite to fix #11131

It consolidates a grown interface with a hard to use enum class Loading that was passed in most cases redundant to the different inputs to a function. To use it right you needed to pass the correct loading value with the correct set of parameters.

This has been now replaces with single purpose functions which should be self-explainable.

static void requestCover() 
static void requestTrackCover()
static QPixmap getCachedCover();
static  void requestUncachedCover();

The original universal interface has been mad private.

To make this work, a refactoring of the migration from legacy hash was required.
This is now always done in the worker thread, to get around the hard to understand extra information of requestedCacheKey and coverInfoUpdated in the CoverFound() signal. This should improve the library scrolling after updating from a legacy Mixxx version.

I have tried hard to do this PR in single testable commits, to allow a good review.

I have two follow up in my stash:

Fix the cover art tooltip if not in cache
Fix the Deck/Spinny Coverart is randomly not read #11131

…okups

Swiftb0y · 2023-09-22T13:03:23Z

src/track/track.cpp

+    static_assert(sizeof(TrackId) <= sizeof(void*)); // no locking required
    return m_record.getId();


why? are you sure the same assumption is true for ARM?

Yes. There is theoretical the issue for a possible odd aliment we may assert as well, but you need to try hard to trick the compiler to achieve that.

In this case, a alignas(int) int m_value; is the best solution without loosing pipelining because of the memory barriers in an a std::atomc_int. I have also remove the generic value_type which gives us the guarantee that we have always int and allows fully in-lining of the DbId class.

I'm still not comfortable with bugprone optimizations like this without evidence that they are needed. I also doubt that a properly used std::atomic would be any slower than this.

I was originally started to implement a std::atomic based solution here. With a 'memory_order_relaxed' you can also achieve that the processor pipeline is not cleared. But than I have noticed that the load a and store calls are (probably intentional) not inline, they are real calls. std::atomic are used for atomic operations like read and increments and memory briers, all this not needed here. We would need a lot of boilerplate code to achieve something with std::atomic that is reliable out of the box.

Here we really just want to read a single integer as a whole that is in addition almost constant. This is already a thing that cannot be interrupted = atomic. The compiler only uses unaligned integers when it is forced to. See also https://stackoverflow.com/a/70967180 and https://learn.microsoft.com/en-us/cpp/cpp/alignment-cpp-declarations?view=msvc-170

I have found a one header implementation of an atomic int that also uses aligneas(int): https://github.com/wingfiring/xirang/blob/eda84647cca476a29e8d785b8fd8ceda78bb952f/trunk/xirang/backward/atomic.h
so we are in a good companion.

But anyway now with alingeas(int) we have sorted out this case as well. So why do you still think this is bugprone. What is the scenario that may fail?

I was originally started to implement a std::atomic based solution here. With a 'memory_order_relaxed' you can also achieve that the processor pipeline is not cleared. But than I have noticed that the load a and store calls are (probably intentional) not inline, they are real calls.

Are you sure, they're fully inlined on -O1 on all compilers. https://compiler-explorer.com/z/n3MbKqr3q
There is no reason why they couldn't be inlined unless atomics where using dynamic dispatch (which they absolutely aren't, considering how performance-sensitive they are).

We would need a lot of boilerplate code to achieve something with std::atomic that is reliable out of the box.

Why would that be? Just make the member atomic and then .load() and .store() to it.

This is already a thing that cannot be interrupted = atomic.

I'm worried the assumption might break, silently resulting in a race-condition. I'd rather have the fallback to slower behavior, than undefined behavior.

I have found a one header implementation of an atomic int that also uses aligneas(int): https://github.com/wingfiring/xirang/blob/eda84647cca476a29e8d785b8fd8ceda78bb952f/trunk/xirang/backward/atomic.h
so we are in a good companion.

That project has been dead for 6 years, I doubt they did any work to validate whether that actually works on every platform they support, which are probably even fewer platform than we support.

But anyway now with alingeas(int) we have sorted out this case as well. So why do you still think this is bugprone. What is the scenario that may fail?

Because on paper alignas just guarantees alignment not atomicity. The conclusion you've drawn that aligned access means atomic access is implicit but not strong enough to not silently break. We either need to be able to assert this (which we can't afaik), or have it documented rigidly for all platforms we support (and might support, I don't want mixxx be unusable on RISC-V because we relied on a bunch of ARM and x86 specific behavior).

If you want to convince me that alignas guarantees atomicity, just link me the section in the standard (or even cppreference would be fine) that supports your assumption. But I don't think that assumption is true in all cases and thus its also not defined in the standard. As such, we can't use it blindly.
If its only defined for some CPU architectures, fine, that works too, but then we need to assert that we only run on those CPU architectures, etc. Do you see the pattern? just linking to some code and stackoverflow articles describing what might happen on some systems doesn't let us draw conclusions over all systems!

Ok I have committed the std::atomic_int version.

Thank you. I will try to take a look, soon-ish.

daschuer · 2023-09-22T15:15:07Z

Is s class always integer alligned? Probably not. I think we need to assert that.

JoergAtGithub · 2023-09-23T13:31:12Z

This doesn't compile here:
src\util\db\dbid.h(27): error C3615: constexpr function 'DbId::DbId' cannot result in a constant expression

JoergAtGithub · 2023-09-23T15:51:48Z

Now I can build locally. Everything works as before, except the Coverart-Tooltip in the library, which does no longer appear. I guess this is intended for this intermediate step.

daschuer · 2023-09-23T16:04:48Z

It should appear after the second try. The first try fills the cache. This should however the same in 2.4.
Can you confirm?

JoergAtGithub · 2023-09-23T16:07:13Z

No, this was the behavior before. With this PR a tooltip for the CoverArt does not appear at all.

daschuer · 2023-09-24T11:39:31Z

Oh, there was missing ! introduced in the last minute. Now everything should be like before, just faster.

uklotzde · 2023-09-25T00:21:03Z

I need to leave a comment here.

DbId is supposed to be a simple, type-safe newtype wrapper around a native type. It is and should not be aware of multiple theads. It is supposed to be used in single-threaded contexts only. A simple POD. All database access is single-threaded.

Now it has become an opinionated AtomicDbId that fits a single use case. But everyone else using this generic type has to pay the price.

uklotzde · 2023-09-25T00:24:31Z

src/util/db/dbid.h

@@ -22,35 +22,36 @@
 // not add any additional state (= member variables). Inheritance is
 // only needed for type-safety.
 class DbId {
-protected:
-public:
-    // Alias for the corresponding native type. It keeps the


The comment explained this intended purpose. But it has been and deleted.

JoergAtGithub · 2023-09-25T05:59:29Z

I tested this, everything works now as before, but the CoverArt loading while scrolling through the library feels to be faster now.

Swiftb0y

blocking this PR for now as I don't agree with the changes. I'll elaborate asap.

daschuer · 2023-09-25T12:04:58Z

I wanted to use the TrackId to check if it is a temporary track or not. However at that point the temporary track used for lookup the metadata is out of scope. So it means the original demand of optimize the locking is not there.

This means we can revert the introduction of an atomic here.

.. in favour of the already existing toString() and toVariant() interface. This hides the internal int value entirely.

daschuer · 2023-09-29T12:25:48Z

@Swiftb0y Ok, the int is gone. Please have a look.

Swiftb0y

can't comment on coverartcache, coverartdelegate and basetracktablemodel, but my concerns over DbIb have been mostly resolved.

src/util/db/dbid.h

can't comment on coverartcache, coverartdelegate and basetracktablemodel as I'm completely unfamiliar with the code in question. Someone else please take over.

daschuer · 2023-11-05T21:28:27Z

The clazy failure is already fixed in 2.4.
Shall I rebase it?

JoergAtGithub · 2023-11-06T20:12:59Z

@daschuer Please fix the build failures - by rebase, or whatever is needed

daschuer · 2023-11-12T10:49:34Z

All green again.

Swiftb0y · 2023-11-12T15:22:44Z

@JoergAtGithub if you're willing to sign off on the coverartcache, coverartdelegate and basetracktablemodel changes, feel free to merge this PR.

JoergAtGithub

I reviewed the code and found only some minor nitpicks. I also build it on Windows 11 and it works as expected.

src/library/coverartcache.cpp

src/library/coverart.h

src/library/basetracktablemodel.cpp

Co-authored-by: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com>

JoergAtGithub

LGTM! Waiting for CI

daschuer · 2023-11-13T06:40:19Z

All green again

JoergAtGithub · 2023-11-13T07:11:24Z

What did you push here?

daschuer · 2023-11-13T07:41:11Z

I have amended the last commit, because there was a code style issue.

daschuer · 2023-11-13T07:43:35Z

Oh no, I see. I have messed with the follow up PTRs.
Need to revert that tonight for a clean history.

daschuer · 2023-11-14T07:12:39Z

Restored the original state from reflog.

JoergAtGithub · 2023-11-14T07:47:53Z

Thank you!

Introduce static CoverArtCache::getCachedCover for immediate cache lo…

a193795

…okups

github-actions bot added code quality library ui labels Sep 22, 2023

daschuer force-pushed the gh11131_2 branch 2 times, most recently from b8445d4 to 40a0c16 Compare September 22, 2023 09:52

Swiftb0y reviewed Sep 22, 2023

View reviewed changes

github-actions bot added the build label Sep 23, 2023

daschuer force-pushed the gh11131_2 branch from 24aa5cf to 90b9af6 Compare September 23, 2023 15:02

daschuer force-pushed the gh11131_2 branch from a6c45da to 50dc762 Compare September 25, 2023 00:01

uklotzde reviewed Sep 25, 2023

View reviewed changes

Swiftb0y requested changes Sep 25, 2023

View reviewed changes

JoergAtGithub mentioned this pull request Sep 25, 2023

fix #11603 table draw delaying the waveformwidgetfactory render and swap #11644

Draft

daschuer added 8 commits September 27, 2023 00:10

Clean up CoverInfo

c8f261d

Perform legacy Hash update in a single place

f9ebc2d

Dispose no longer used requestedImageHash and coverInfoUpdated

0559fac

Introduce CoverArtCache::requestUncachedCover()

a3e802b

Bypass redundant cache lookup when using "Uncached" interface

00bd5dd

Use fixed typed int for copletely inlined implementation

d648d2c

Fix a race condition in getLocation()

2d0855f

Improve condition for updating legacy hashes.

9b13045

Dispose unused DbId(int value) and DbId::value()

92c19ac

.. in favour of the already existing toString() and toVariant() interface. This hides the internal int value entirely.

daschuer requested a review from Swiftb0y October 3, 2023 22:07

This was referenced Oct 11, 2023

Fix coverart tootip if cover is not cached #12087

Merged

Fix reading the Spinny cover on Windows #12103

Merged

daschuer added this to the 2.4.0 milestone Oct 14, 2023

Swiftb0y reviewed Nov 2, 2023

View reviewed changes

src/util/db/dbid.h Outdated Show resolved Hide resolved

JoergAtGithub requested a review from Swiftb0y November 5, 2023 11:39

delete DbId(int) to make sure we have no extra in roundtrip

988432c

daschuer force-pushed the gh11131_2 branch from b3eca17 to 988432c Compare November 12, 2023 09:47

JoergAtGithub requested changes Nov 12, 2023

View reviewed changes

Improve code readability

c8f308d

Co-authored-by: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com>

daschuer force-pushed the gh11131_2 branch from 8873ce9 to 2f956ee Compare November 12, 2023 22:17

JoergAtGithub approved these changes Nov 12, 2023

View reviewed changes

Manual inline requestUncachedCoverPrivate()

d5c5f1c

daschuer force-pushed the gh11131_2 branch from 2f956ee to d5c5f1c Compare November 12, 2023 23:15

daschuer force-pushed the gh11131_2 branch from d5c5f1c to 067b7a6 Compare November 13, 2023 06:45

daschuer force-pushed the gh11131_2 branch from 067b7a6 to d5c5f1c Compare November 14, 2023 07:01

JoergAtGithub merged commit a09236f into mixxxdj:2.4 Nov 14, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CoverArtCache refactoring + Fix scrolling lag after updating Mixxx #12009

CoverArtCache refactoring + Fix scrolling lag after updating Mixxx #12009

daschuer commented Sep 22, 2023

Swiftb0y Sep 22, 2023

daschuer Sep 22, 2023

daschuer Sep 23, 2023

Swiftb0y Sep 23, 2023

daschuer Sep 24, 2023 •

edited

Swiftb0y Sep 24, 2023

Swiftb0y Sep 24, 2023

daschuer Sep 24, 2023

Swiftb0y Sep 25, 2023

daschuer commented Sep 22, 2023 •

edited

JoergAtGithub commented Sep 23, 2023

JoergAtGithub commented Sep 23, 2023

daschuer commented Sep 23, 2023

JoergAtGithub commented Sep 23, 2023

daschuer commented Sep 24, 2023

uklotzde commented Sep 25, 2023 •

edited

uklotzde Sep 25, 2023 •

edited

JoergAtGithub commented Sep 25, 2023

Swiftb0y left a comment

daschuer commented Sep 25, 2023

daschuer commented Sep 29, 2023

Swiftb0y left a comment

daschuer commented Nov 5, 2023

JoergAtGithub commented Nov 6, 2023

daschuer commented Nov 12, 2023

Swiftb0y commented Nov 12, 2023

JoergAtGithub left a comment

JoergAtGithub left a comment

daschuer commented Nov 13, 2023

JoergAtGithub commented Nov 13, 2023

daschuer commented Nov 13, 2023

daschuer commented Nov 13, 2023 •

edited

daschuer commented Nov 14, 2023

JoergAtGithub commented Nov 14, 2023

		static_assert(sizeof(TrackId) <= sizeof(void*)); // no locking required
		return m_record.getId();

CoverArtCache refactoring + Fix scrolling lag after updating Mixxx #12009

CoverArtCache refactoring + Fix scrolling lag after updating Mixxx #12009

Conversation

daschuer commented Sep 22, 2023

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 Sep 24, 2023 • edited

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 commented Sep 22, 2023 • edited

JoergAtGithub commented Sep 23, 2023

JoergAtGithub commented Sep 23, 2023

daschuer commented Sep 23, 2023

JoergAtGithub commented Sep 23, 2023

daschuer commented Sep 24, 2023

uklotzde commented Sep 25, 2023 • edited

uklotzde Sep 25, 2023 • edited

Choose a reason for hiding this comment

JoergAtGithub commented Sep 25, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Sep 25, 2023

daschuer commented Sep 29, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer commented Nov 5, 2023

JoergAtGithub commented Nov 6, 2023

daschuer commented Nov 12, 2023

Swiftb0y commented Nov 12, 2023

JoergAtGithub left a comment

Choose a reason for hiding this comment

JoergAtGithub left a comment

Choose a reason for hiding this comment

daschuer commented Nov 13, 2023

JoergAtGithub commented Nov 13, 2023

daschuer commented Nov 13, 2023

daschuer commented Nov 13, 2023 • edited

daschuer commented Nov 14, 2023

JoergAtGithub commented Nov 14, 2023

daschuer Sep 24, 2023 •

edited

daschuer commented Sep 22, 2023 •

edited

uklotzde commented Sep 25, 2023 •

edited

uklotzde Sep 25, 2023 •

edited

daschuer commented Nov 13, 2023 •

edited