D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Sune Vuorela]

svuorela added a comment.


  @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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:d2d52da38016: Avoid QByteArray::remove in 
AccessManagerReply::readData (authored by fvogt).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15426?vs=41476=42138

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt added a comment.


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

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Sune Vuorela]

svuorela accepted this revision.
svuorela added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio, dfaure, svuorela
Cc: svuorela, ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Luca Beltrame]

lbeltrame added a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio, dfaure
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Stefan Brüns]

bruns added a comment.


  LGTM

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt updated this revision to Diff 41476.
fvogt added a comment.


  Remove code.
  
  Any more nits to pick?

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15426?vs=41468=41476

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt added a comment.


  In D15426#324636 , @bruns wrote:
  
  > In D15426#324486 , @fvogt wrote:
  >
  > > In D15426#324284 , @bruns 
wrote:
  > >
  > > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Stefan Brüns]

bruns added a comment.


  In D15426#324486 , @fvogt wrote:
  
  > In D15426#324284 , @bruns wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt updated this revision to Diff 41468.
fvogt added a comment.


  The empty "if" is kept for readability.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15426?vs=41456=41468

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> accessmanagerreply_p.cpp:433
> +newData.append(data);
> +m_data = newData;
> +m_offset = 0;

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt updated this revision to Diff 41456.
fvogt added a comment.


  Avoid reallocation in slotData if removing m_offset frees enough space.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15426?vs=41455=41456

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt added a comment.


  In D15426#324284 , @bruns wrote:
  
  > 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`.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt updated this revision to Diff 41455.
fvogt added a comment.


  Save m_offset bytes in slotData as well.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15426?vs=41427=41455

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Stefan Brüns]

bruns added a comment.


  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`.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> bruns wrote in accessmanagerreply_p.cpp:156
> Wouldn't it be better to trim the buffer in slotData, at least in the 
> non-trivial case?

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.

> bruns wrote in accessmanagerreply_p.h:99
> You could use in-class initialization here ...

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> accessmanagerreply_p.cpp:156
>  
> -if (length) {
> -memcpy(data, m_data.constData(), length);
> -m_data.remove(0, length);
> +// Remove the stale data before m_offset only if it grows to half
> +// of the total size, to avoid calling the expensive .remove function.

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

> accessmanagerreply_p.h:99
>  QByteArray m_data;
> +qint64 m_offset;
>  bool m_metaDataRead;

You could use in-class initialization here ...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt updated this revision to Diff 41427.
fvogt added a comment.


  - Actually make it work
  - Free memory if m_offset is half of the array size
  - Bail out if maxSize < 0

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15426?vs=41406=41427

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt edited the summary of this revision.
fvogt edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15426

To: fvogt, #frameworks, elvisangelaccio
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15426: Avoid QByteArray::remove in AccessManagerReply::readData

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Frameworks, elvisangelaccio.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  It copies the remaining data to the beginning of the buffer, which can be
  really expensive.
  BUG: 375765

TEST PLAN
  Only build tested, please test with kio-gdrive.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15426

AFFECTED FILES
  src/widgets/accessmanagerreply_p.cpp
  src/widgets/accessmanagerreply_p.h

To: fvogt, #frameworks, elvisangelaccio
Cc: kde-frameworks-devel, michaelh, ngraham, bruns