Differential D8279

Delay purging of KisTextureTileInfoPoolSingleSize
ClosedPublic
Actions

Authored by poke1024 on Oct 13 2017, 9:02 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Commits
R37:c998e136c3b1: Delay purging of KisTextureTileInfoPoolSingleSize
R37:9547c8d0e3b2: Delay purging of KisTextureTileInfoPoolSingleSize
Summary

This is a follow up of the observations in D8033.

Purging of KisOpenGLUpdateInfos in KisTextureTileInfoPoolSingleSize seems to happen very often on my system. I'm not sure if this is something specific I only observe here. What seems to happen is that the queue gets completely empty, gets purged, only then to be reallocated. This happens 10+ times per second (see test plan below).

I can see this in the profiling as well (originally observed in D8033):

Here, the KisOpenGLUpdateInfo destructor alone takes 13% inside the main thread (namely 4.3% of the 33.0% of the main thread).

With this patch, the profiling impact of the KisOpenGLUpdateInfo destructor drops to < 1% (0.3% of 33.7%).

I found it quite difficult to get out of the multithread context into a safe main thread QTimer thing. Maybe there's a simpler way than what I came up with?

Test Plan

Turn on debugging output for the purging in KisTextureTileInfoPoolSingleSize::free(). On my system, I can see that purges happen many times per second, e.g.:

"21:14:59"  purging memory m_maxAllocations = 99
"21:14:59"  purging memory m_maxAllocations = 83
"21:14:59"  purging memory m_maxAllocations = 178
"21:14:59"  purging memory m_maxAllocations = 104
"21:15:00"  purging memory m_maxAllocations = 104
"21:15:00"  purging memory m_maxAllocations = 72
"21:15:00"  purging memory m_maxAllocations = 67
"21:15:00"  purging memory m_maxAllocations = 97
"21:15:00"  purging memory m_maxAllocations = 98
"21:15:00"  purging memory m_maxAllocations = 159
"21:15:00"  purging memory m_maxAllocations = 93
"21:15:00"  purging memory m_maxAllocations = 75
"21:15:00"  purging memory m_maxAllocations = 70
"21:15:00"  purging memory m_maxAllocations = 65
"21:15:00"  purging memory m_maxAllocations = 128
"21:15:00"  purging memory m_maxAllocations = 92
"21:15:00"  purging memory m_maxAllocations = 99
"21:15:00"  purging memory m_maxAllocations = 98
"21:15:00"  purging memory m_maxAllocations = 71
"21:15:00"  purging memory m_maxAllocations = 97
"21:15:00"  purging memory m_maxAllocations = 77
"21:15:00"  purging memory m_maxAllocations = 99
"21:15:00"  purging memory m_maxAllocations = 91
"21:15:00"  purging memory m_maxAllocations = 97
"21:15:00"  purging memory m_maxAllocations = 73
"21:15:00"  purging memory m_maxAllocations = 97
"21:15:00"  purging memory m_maxAllocations = 81
"21:15:00"  purging memory m_maxAllocations = 86
"21:15:00"  purging memory m_maxAllocations = 110
"21:15:01"  purging memory m_maxAllocations = 101
"21:15:01"  purging memory m_maxAllocations = 129
"21:15:01"  purging memory m_maxAllocations = 138
"21:15:01"  purging memory m_maxAllocations = 92
"21:15:01"  purging memory m_maxAllocations = 112
"21:15:01"  purging memory m_maxAllocations = 69
"21:15:01"  purging memory m_maxAllocations = 111
"21:15:01"  purging memory m_maxAllocations = 77
"21:15:01"  purging memory m_maxAllocations = 74
"21:15:01"  purging memory m_maxAllocations = 96
"21:15:01"  purging memory m_maxAllocations = 72

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.
poke1024 created this revision.Oct 13 2017, 9:02 PM
rempt added a reviewer: dkazakov.Oct 14 2017, 8:55 AM
rempt added a subscriber: rempt.

I don't see any regressions, so this is probably fine, but let's wait for Dmitry's input.

libs/ui/opengl/kis_texture_tile_info_pool.cpp
3

Is this yours or Dmitry's?

libs/ui/opengl/kis_texture_tile_info_pool.h
60

I'm not totally sure about the naming of this variable,

poke1024 updated this revision to Diff 21123.Oct 22 2017, 11:33 AM

Renames version to numFrees which is a more accessible name.

This variable's idea is to prevent the following (academic) situation:

(1) pool is empty, triggers delayed purge
(2) new allocations and deallocations happen
(3) trigger from (1) gets called. checks pool and it is empty even though the pool was busy in (2) with new allocations and deallocations. the assumption that the pool is no longer in use and thus purgeable is not valid therefore.

Fixes author name.

poke1024 marked an inline comment as done.Oct 22 2017, 11:34 AM
poke1024 added inline comments.
libs/ui/opengl/kis_texture_tile_info_pool.h
60

Renamed to numFrees.

dkazakov accepted this revision.Oct 23 2017, 8:26 AM

Hi, @poke1024!

The patch looks correct and works perfectly fine. Please fix two minor comments I added inline and push the patch.

libs/ui/opengl/kis_texture_tile_info_pool.h
93

Don't forget to remove debugging lines before pushing to master :)

121

Please use KisSignalCompressor in POSTPONE mode instead of the raw QTimer. It is more maintainable and configurable.

If you don't want the header make the including user depend on a custom class, just add the compressor as a pointer and make the worker its parent, and QObject will delete it automatically on destruction.

This revision is now accepted and ready to land.Oct 23 2017, 8:26 AM
poke1024 updated this revision to Diff 21170.Oct 23 2017, 11:23 AM
poke1024 marked 2 inline comments as done.

Switched from QTimer to KisSignalCompressor.

dkazakov accepted this revision.Oct 23 2017, 11:25 AM

Please push! :)

poke1024 added a comment.Oct 23 2017, 12:34 PM

Hi @dkazakov, I applied the changes, but during final testing I found that the code currently does not free anything, as after changing the texture size to 2048, the freeThreshold of 64 is never reached (the number of allocated tiles is now around 7 to 10 in my tests). Lowering freeThreshold to 8 enables purging again, but this is really a problem related to freeThreshold and texture size, not this PR.

In other words, things are broken in the top of three and not with this PR. I think we need to address changing freeThreshold and the other constants or rolling back the texture size change in the context of D8280.

So I suggest I still push this, and we fix the constants/texture size in a different ticket?

dkazakov added a comment.Oct 23 2017, 1:56 PM

Hi, @poke1024!

I've just noticed the texture size patch... Increasing the default texture size is really a bad idea. I just tested it on my GPU (nvidia) and it causes an extreme lag and hangups when trying to paint with 30px brush on 10000x8000 px canvas.

So I would just recommend to revert the texture size patch and push this one instead. Having a flexible pool size is a good idea though, but not a priority.

Closed by commit R37:c998e136c3b1: Delay purging of KisTextureTileInfoPoolSingleSize (authored by poke1024). · Explain WhyOct 23 2017, 2:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 21181

View Options

libs/ui/CMakeLists.txt

Loading...
View Options

libs/ui/opengl/kis_texture_tile_info_pool.h

Loading...
View Options

libs/ui/opengl/kis_texture_tile_info_pool.cpp

Loading...