Reduce temporary allocations in the DesktopFileParser
Review Request #128944 - Created Sept. 19, 2016 and discarded
|Aleix Pol Gonzalez|
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.
tests still pass, plasma still works normally.
leaked allocations: 83225
temporary allocations: 606902
leaked allocations: 84825
temporary allocations: 819292
|if this compiles before and after, then you should set the defines that prevent implicit string conversions and fix all ...||Milian Wolff|
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.
I guess the same optimization could be applied here.
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?
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?
line.midRef(0).trimmed()should work without any extra allocations, right?
We can remove valueEscaped now that both are of type QString
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 :)
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?