Avoid QByteArray::remove in AccessManagerReply::readData
ClosedPublic

Authored by fvogt on Sep 11 2018, 1:27 PM.

Details

Summary

If m_data contains a relatively large amount of data, calls to readData are
really expensive as it copies the remaining data around in memory.

For some reason, readAll() on an AccessManagerReply results in calls to
readData with a maxSize of 16384 until it returns 0. That's pretty much the
worst case scenario, but also the most likely used one.

Avoid the expensive remove operation by only calling remove if it saves
sufficient memory.

CCBUG: 375765

Additionally, bail out if maxSize is < 0.

Test Plan

Ran accessmanagertest, passes.
Downloaded files with kio-gdrive, not stuck in memmove anymore.
It still dies for very large files, but that's a design issue in kio-gdrive
which can't be addressed in KIO.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Sep 11 2018, 1:27 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 11 2018, 1:27 PM
fvogt requested review of this revision.Sep 11 2018, 1:27 PM
fvogt added a comment.Sep 11 2018, 1:33 PM

Depending on how AccessManagerReply is used, it might be necessary to do m_data.remove(0, m_offset); m_offset = 0; if m_offset grows too large to not leak memory. Can a KIO expert answer this?

fvogt edited the summary of this revision. (Show Details)Sep 11 2018, 5:33 PM
fvogt edited the test plan for this revision. (Show Details)
fvogt updated this revision to Diff 41427.Sep 11 2018, 5:34 PM
  • Actually make it work
  • Free memory if m_offset is half of the array size
  • Bail out if maxSize < 0
fvogt edited the summary of this revision. (Show Details)Sep 11 2018, 5:41 PM
bruns added a subscriber: bruns.Sep 11 2018, 6:47 PM
bruns added inline comments.
src/widgets/accessmanagerreply_p.cpp
158–160

Wouldn't it be better to trim the buffer in slotData, at least in the non-trivial case?

src/widgets/accessmanagerreply_p.h
99

You could use in-class initialization here ...

fvogt added inline comments.Sep 11 2018, 6:51 PM
src/widgets/accessmanagerreply_p.cpp
158–160

For the non-trivial case it shouldn't make a difference really. For the trivial one it does, as otherwise it would never empty m_data.

So to avoid code duplication, it's only done here.

src/widgets/accessmanagerreply_p.h
99

It's not done for the other members. I assume it's a style preference.

bruns added a comment.Sep 11 2018, 8:23 PM

For the trivial case, do the clear in readData().

For the non-trivial case:

  1. in readData(), no memmoves were ever done. Currently, if you have e.g. 50 kB in m_data, you read 2 * 16 kB, and move the remaining 18 kB to the front. You can instead just read 3 * 16 + 2 kB.
  2. in slotData(), you have to do a copy every time capacity() is exceeded. You can piggy-pack on this necessary copy/move, and either do a move if m_offset >= data.size() else do a copy of the old data, last append the new data.
fvogt updated this revision to Diff 41455.Sep 12 2018, 7:57 AM

Save m_offset bytes in slotData as well.

fvogt added a comment.Sep 12 2018, 8:01 AM

For the trivial case, do the clear in readData().

For the non-trivial case:

  1. in readData(), no memmoves were ever done. Currently, if you have e.g. 50 kB in m_data, you read 2 * 16 kB, and move the remaining 18 kB to the front. You can instead just read 3 * 16 + 2 kB.

It's always a cpu/memory trade off. I prefer KISS to premature optimization.

  1. in slotData(), you have to do a copy every time capacity() is exceeded. You can piggy-pack on this necessary copy/move, and either do a move if m_offset >= data.size() else do a copy of the old data, last append the new data.

Sure, I added that. Note that it does not actually make a difference for the readAll() case as m_offset is always 0 in slotData.

fvogt updated this revision to Diff 41456.Sep 12 2018, 8:19 AM

Avoid reallocation in slotData if removing m_offset frees enough space.

bruns added inline comments.Sep 12 2018, 11:28 AM
src/widgets/accessmanagerreply_p.cpp
431

if you swap these two lines (i.e. append to m_data), you have m_data += data in all three branches, and you can move it to the bottom.

fvogt updated this revision to Diff 41468.Sep 12 2018, 12:12 PM

The empty "if" is kept for readability.

fvogt marked 3 inline comments as done.Sep 12 2018, 12:13 PM
bruns added a comment.Sep 12 2018, 1:37 PM

For the trivial case, do the clear in readData().

For the non-trivial case:

  1. in readData(), no memmoves were ever done. Currently, if you have e.g. 50 kB in m_data, you read 2 * 16 kB, and move the remaining 18 kB to the front. You can instead just read 3 * 16 + 2 kB.

It's always a cpu/memory trade off. I prefer KISS to premature optimization.

if you remove the || m_offset * 2 >= m_data.length(), it gets even simpler, and does less work (especially for the readAll() case). The garbage collection is already done in slotData.

fvogt added a comment.Sep 12 2018, 1:42 PM

For the trivial case, do the clear in readData().

For the non-trivial case:

  1. in readData(), no memmoves were ever done. Currently, if you have e.g. 50 kB in m_data, you read 2 * 16 kB, and move the remaining 18 kB to the front. You can instead just read 3 * 16 + 2 kB.

It's always a cpu/memory trade off. I prefer KISS to premature optimization.

if you remove the || m_offset * 2 >= m_data.length(), it gets even simpler, and does less work (especially for the readAll() case). The garbage collection is already done in slotData.

I did not remove that because it means even if there's only a single byte in the buffer remaining (for some reason), the memory of m_data is never freed.

However, I looked at the QByteArray source code and found that :remove doesn't actually change the capacity of the array at all, so it's indeed a bit pointless.

fvogt updated this revision to Diff 41476.Sep 12 2018, 1:54 PM

Remove code.

Any more nits to pick?

svuorela accepted this revision.Sep 22 2018, 4:19 PM
svuorela added a subscriber: svuorela.

Though unit tests would be nice. It looks like something that quite easy could break if next author is not careful.

This revision is now accepted and ready to land.Sep 22 2018, 4:19 PM
fvogt added a comment.Sep 22 2018, 4:21 PM

@svuorela accessmanagertest already tests whether the AMR works in general. Do you mean a test which ensures there is no performance regression?

This revision was automatically updated to reflect the committed changes.

@fvogt mostly a unit test that ensures that all these 3 codepaths in slotData works. I'm not sure that the current unit tests ensures that. I might be wrong though.

So this doesn't completely fix https://bugs.kde.org/show_bug.cgi?id=375765? More is needed?