Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/
---

(Updated Oct. 26, 2016, 2:54 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks.


Repository: kcoreaddons


Description
---

While analising plasmashell under heaptrack, one of the sore spots is temporary 
allocations within DesktopFileParser. This improves the situation by:

* Only converting to QString/utf8 once.
* Using QStringRef instead of fully splitting QString to parse them.


Diffs
-

  src/lib/plugin/desktopfileparser.cpp 2eb198d 
  src/lib/plugin/desktopfileparser_p.h c61b297 

Diff: https://git.reviewboard.kde.org/r/128944/diff/


Testing
---

tests still pass, plasma still works normally.

heaptrack plasmashell:

after:
allocations:4169312
leaked allocations: 83225
temporary allocations:  606902

before:
allocations:4680691
leaked allocations: 84825
temporary allocations:  819292


Thanks,

Aleix Pol Gonzalez

Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Milian Wolff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/#review99391
---



personally, I don't think it's a good idea to use QString instead of QByteArray 
here. If that is really just for QStringRef in the function, wouldn't a simple 
view class on the byte array be the better choice? It's not much code to write, 
afaik you could even copy the code that is on gerrit somewhere for 
QByteArrayView. That way you will reduce the allocations but keep the data in 
UTF-8 instead of converting it to UTF-16. Also, your patch will probably use 
more memory now, or?


src/lib/plugin/desktopfileparser.cpp (line 99)


famous last words ;-)


- Milian Wolff


On Sept. 20, 2016, 11:19 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128944/
> ---
> 
> (Updated Sept. 20, 2016, 11:19 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> While analising plasmashell under heaptrack, one of the sore spots is 
> temporary allocations within DesktopFileParser. This improves the situation 
> by:
> 
> * Only converting to QString/utf8 once.
> * Using QStringRef instead of fully splitting QString to parse them.
> 
> 
> Diffs
> -
> 
>   src/lib/plugin/desktopfileparser.cpp 2eb198d 
>   src/lib/plugin/desktopfileparser_p.h c61b297 
> 
> Diff: https://git.reviewboard.kde.org/r/128944/diff/
> 
> 
> Testing
> ---
> 
> tests still pass, plasma still works normally.
> 
> heaptrack plasmashell:
> 
> after:
> allocations:4169312
> leaked allocations: 83225
> temporary allocations:  606902
> 
> before:
> allocations:4680691
> leaked allocations: 84825
> temporary allocations:  819292
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/
---

(Updated Sept. 20, 2016, 1:19 p.m.)


Review request for KDE Frameworks.


Repository: kcoreaddons


Description
---

While analising plasmashell under heaptrack, one of the sore spots is temporary 
allocations within DesktopFileParser. This improves the situation by:

* Only converting to QString/utf8 once.
* Using QStringRef instead of fully splitting QString to parse them.


Diffs (updated)
-

  src/lib/plugin/desktopfileparser.cpp 2eb198d 
  src/lib/plugin/desktopfileparser_p.h c61b297 

Diff: https://git.reviewboard.kde.org/r/128944/diff/


Testing
---

tests still pass, plasma still works normally.

heaptrack plasmashell:

after:
allocations:4169312
leaked allocations: 83225
temporary allocations:  606902

before:
allocations:4680691
leaked allocations: 84825
temporary allocations:  819292


Thanks,

Aleix Pol Gonzalez

Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Kai Uwe Broulik]

> On Sept. 19, 2016, 8:08 nachm., Alex Richardson wrote:
> > src/lib/plugin/desktopfileparser.cpp, line 477
> > 
> >
> > Using readLineInto() is really useful to reduce unnecessary 
> > allocations. However the function is quite new: `This function was 
> > introduced in Qt 5.5`
> > 
> > Do we depend on Qt 5.5 already?

set(REQUIRED_QT_VERSION 5.5.0) says kcoreaddons CMakeLists.txt


- Kai Uwe


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/#review99278
---


On Sept. 19, 2016, 3:20 nachm., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128944/
> ---
> 
> (Updated Sept. 19, 2016, 3:20 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> While analising plasmashell under heaptrack, one of the sore spots is 
> temporary allocations within DesktopFileParser. This improves the situation 
> by:
> 
> * Only converting to QString/utf8 once.
> * Using QStringRef instead of fully splitting QString to parse them.
> 
> 
> Diffs
> -
> 
>   src/lib/plugin/desktopfileparser.cpp 2eb198d 
>   src/lib/plugin/desktopfileparser_p.h c61b297 
> 
> Diff: https://git.reviewboard.kde.org/r/128944/diff/
> 
> 
> Testing
> ---
> 
> tests still pass, plasma still works normally.
> 
> heaptrack plasmashell:
> 
> after:
> allocations:4169312
> leaked allocations: 83225
> temporary allocations:  606902
> 
> before:
> allocations:4680691
> leaked allocations: 84825
> temporary allocations:  819292
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Mark Gaiser]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/#review99302
---



I think you might be better off (and faster) if you use 
http://doc.qt.io/qt-5/qbytearray.html#fromRawData which smells like a 
"QByteArrayRef". That saves you the conversion from QByteArray to QString (you 
only need to do that where you actually need it). Try it, you might save more 
reallocations this way :)


src/lib/plugin/desktopfileparser.cpp (line 280)


You can optimize this one further.
http://doc.qt.io/qt-5/qbytearray.html#fromRawData

You'd just have to figure out the first position from where you want to 
have the string. QByteArray::lastIndexOf(...) probably helps you there.


- Mark Gaiser


On sep 19, 2016, 3:20 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128944/
> ---
> 
> (Updated sep 19, 2016, 3:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> While analising plasmashell under heaptrack, one of the sore spots is 
> temporary allocations within DesktopFileParser. This improves the situation 
> by:
> 
> * Only converting to QString/utf8 once.
> * Using QStringRef instead of fully splitting QString to parse them.
> 
> 
> Diffs
> -
> 
>   src/lib/plugin/desktopfileparser.cpp 2eb198d 
>   src/lib/plugin/desktopfileparser_p.h c61b297 
> 
> Diff: https://git.reviewboard.kde.org/r/128944/diff/
> 
> 
> Testing
> ---
> 
> tests still pass, plasma still works normally.
> 
> heaptrack plasmashell:
> 
> after:
> allocations:4169312
> leaked allocations: 83225
> temporary allocations:  606902
> 
> before:
> allocations:4680691
> leaked allocations: 84825
> temporary allocations:  819292
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Alex Richardson]

> On Sept. 19, 2016, 9:08 p.m., Alex Richardson wrote:
> > src/lib/plugin/desktopfileparser.cpp, line 478
> > 
> >
> > If I read this correctly we no longer handle leading/trailing spaces 
> > properly. Does the test still pass? Or maybe I forgot to add tests for this 
> > case.
> > trimmed() will add another allocation, maybe we can change the string 
> > in place?
> > 
> > There is `QStringRef::trimmed()` so `line.midRef(0).trimmed()` should 
> > work without any extra allocations, right?

`leftRef(-1)` is slightly more efficient so rather use that I guess


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/#review99278
---


On Sept. 19, 2016, 4:20 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128944/
> ---
> 
> (Updated Sept. 19, 2016, 4:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> While analising plasmashell under heaptrack, one of the sore spots is 
> temporary allocations within DesktopFileParser. This improves the situation 
> by:
> 
> * Only converting to QString/utf8 once.
> * Using QStringRef instead of fully splitting QString to parse them.
> 
> 
> Diffs
> -
> 
>   src/lib/plugin/desktopfileparser.cpp 2eb198d 
>   src/lib/plugin/desktopfileparser_p.h c61b297 
> 
> Diff: https://git.reviewboard.kde.org/r/128944/diff/
> 
> 
> Testing
> ---
> 
> tests still pass, plasma still works normally.
> 
> heaptrack plasmashell:
> 
> after:
> allocations:4169312
> leaked allocations: 83225
> temporary allocations:  606902
> 
> before:
> allocations:4680691
> leaked allocations: 84825
> temporary allocations:  819292
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Re: Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Alex Richardson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/#review99278
---



Thanks for looking into this!

When I wrote the code I thought it would be mostly transitional so I didn't pay 
much attention to efficiency.


src/lib/plugin/desktopfileparser.cpp (line 215)


I guess the same optimization could be applied here.



src/lib/plugin/desktopfileparser.cpp (line 477)


Using readLineInto() is really useful to reduce unnecessary allocations. 
However the function is quite new: `This function was introduced in Qt 5.5`

Do we depend on Qt 5.5 already?



src/lib/plugin/desktopfileparser.cpp (line 478)


If I read this correctly we no longer handle leading/trailing spaces 
properly. Does the test still pass? Or maybe I forgot to add tests for this 
case.
trimmed() will add another allocation, maybe we can change the string in 
place?

There is `QStringRef::trimmed()` so `line.midRef(0).trimmed()` should work 
without any extra allocations, right?



src/lib/plugin/desktopfileparser.cpp (line 506)


We can remove valueEscaped now that both are of type QString


- Alex Richardson


On Sept. 19, 2016, 4:20 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128944/
> ---
> 
> (Updated Sept. 19, 2016, 4:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> While analising plasmashell under heaptrack, one of the sore spots is 
> temporary allocations within DesktopFileParser. This improves the situation 
> by:
> 
> * Only converting to QString/utf8 once.
> * Using QStringRef instead of fully splitting QString to parse them.
> 
> 
> Diffs
> -
> 
>   src/lib/plugin/desktopfileparser.cpp 2eb198d 
>   src/lib/plugin/desktopfileparser_p.h c61b297 
> 
> Diff: https://git.reviewboard.kde.org/r/128944/diff/
> 
> 
> Testing
> ---
> 
> tests still pass, plasma still works normally.
> 
> heaptrack plasmashell:
> 
> after:
> allocations:4169312
> leaked allocations: 83225
> temporary allocations:  606902
> 
> before:
> allocations:4680691
> leaked allocations: 84825
> temporary allocations:  819292
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

Review Request 128944: Reduce temporary allocations in the DesktopFileParser

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128944/
---

Review request for KDE Frameworks.


Repository: kcoreaddons


Description
---

While analising plasmashell under heaptrack, one of the sore spots is temporary 
allocations within DesktopFileParser. This improves the situation by:

* Only converting to QString/utf8 once.
* Using QStringRef instead of fully splitting QString to parse them.


Diffs
-

  src/lib/plugin/desktopfileparser.cpp 2eb198d 
  src/lib/plugin/desktopfileparser_p.h c61b297 

Diff: https://git.reviewboard.kde.org/r/128944/diff/


Testing
---

tests still pass, plasma still works normally.

heaptrack plasmashell:

after:
allocations:4169312
leaked allocations: 83225
temporary allocations:  606902

before:
allocations:4680691
leaked allocations: 84825
temporary allocations:  819292


Thanks,

Aleix Pol Gonzalez