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

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-framewo

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

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,

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

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&id=42138 REVI

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

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, elvisan

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

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://pha

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

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

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

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&id=41476 BRANCH master REVISION DETAIL https://phabricator.kde.org/D15426 AFFECTED FI

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

fvogt added a comment. In D15426#324636 , @bruns wrote: > In D15426#324486 , @fvogt wrote: > > > In D15426#324284 , @bruns wrote: > > > > > For the triv

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

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

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

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

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&id=41468 BRANCH master REVISION DETAIL https://phabricator.kde.org/D15426 AFFECTED FIL

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

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 bot

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

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&id=41456 BRANCH master REVISION DETAIL https://phabrica

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

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, y

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

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&id=41455 BRANCH master REVISION DETAIL https://phabricator.kde.org/D15426 AFFECTED FI

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

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

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

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 wo

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

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 a

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

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

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&id=41427 BRANCH master

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

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

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/D15

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

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