Rewrite for KoColorSpaceRegistry to fix multithreading and deadlocks
ClosedPublic

Authored by dkazakov on May 11 2017, 11:09 AM.

Details

Summary

Basically, we have two levels of locks in the registry:

  1. (outer level) is Private::registrylock, which controls the structures of the color space registry itself
  2. (inner level) is KoColorProfileStorage::Private::lock controls the structures related to profiles.

The locks can be taken individually, but if you are going to take both
of them, you should always follow the order 1) registry; 2) profiles.
Otherwise you'll get a deadlock.

All the dependent classes (KoColorConversionSystem and KoColorSpaceFactory)
now do not use the direct links to the registry. Instead, they use special
private interfaces that skip recursive locking and ensure we don't have
deadlocks.

Also includes two commits:

  • Add -Werror=delete-incomplete
  • Fix KisProjectionBenchmark to handle threading properly
Test Plan

Just use Krita in normal way with different color spaces and using different filters.

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dkazakov created this revision.May 11 2017, 11:09 AM
This revision was automatically updated to reflect the committed changes.
dkazakov reopened this revision.May 11 2017, 12:19 PM

It is not closed. It is just pushed into a separate branch.

dkazakov updated this revision to Diff 14400.May 11 2017, 12:23 PM

Uploaded the correct diff

dkazakov edited the summary of this revision. (Show Details)May 11 2017, 12:23 PM
rempt accepted this revision.May 15 2017, 12:37 PM

Two remarks -- other than that, i guess I'm fine with it. I've built and tested it.

libs/global/KisReadWriteLockPolicy.h
33 ↗(On Diff #14400)

This one isn't used anywhere?

libs/pigment/KoColorProfileStorage.h
29 ↗(On Diff #14400)

Needs dox, I'd say.

This revision is now accepted and ready to land.May 15 2017, 12:37 PM

Reported a regression:

15:43 < nicholasl> dmitryk|log: If I recall, after your commit "Rewrite alpha colorspaces using templates and make them correct", it became possible to create a new document with a color space of 
                   "Alpha". In such a case, Krita would simply crash.

Ok, seems like Alpha crash in unrelated. So I'll just merge this.

dkazakov marked 2 inline comments as done.May 18 2017, 10:42 AM

Ok, I added the dox locally, so I'll just merge the updated version,

libs/global/KisReadWriteLockPolicy.h
33 ↗(On Diff #14400)

I wanted this class quite a lot of time, but was always too lazy and used manual unlock() and lockForWrite(). This class will help us avoid this in the future. Next time I face such a problem I'll try to look through all the instances of manual relocking.

This revision was automatically updated to reflect the committed changes.
dkazakov marked an inline comment as done.