D26126: Simplify param method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

David,

Thanks for the detailed explanation, I'm currently reworking most of the
patches here. About the optimizations - I know the linear search is a bad
optimization, but it adds legibility, that's what I tried to do.
I tried to emulate the python `if value in vector` that is really easy to
read, compared to the current code.



On Tue, Dec 24, 2019 at 9:40 AM David Faure <noreply at phabricator.kde.org>
wrote:

> dfaure requested changes to this revision.
> dfaure added inline comments. View Revision
> <https://phabricator.kde.org/D26126>
> *INLINE COMMENTS*
> View Inline <https://phabricator.kde.org/D26126#inline-147537>
> kconfig_compiler.cpp:1020
> } else if (type == QLatin1String("font")) {
> return QStringLiteral("const QFont &");
> } else if (type == QLatin1String("rect")) {
> QLatin1String("double")}
> ).contains(type)) {
> return type;
>
> This linear search doesn't look like an optimization to me. It would be
> better to incorporate this into the map, so that a single search is
> performed, rather than two.
>
> View Inline <https://phabricator.kde.org/D26126#inline-147473>ervin wrote
> in kconfig_compiler.cpp:1024
>
> std::map looks like a bad idea here, either use QHash (preferred in
> massively based Qt code) or std::unordered_map.
>
> Also for both temporaries you pay the price of their allocation and
> deallocation at each call. Even a mutex used at each call would do better.
> ;-)
>
> I'm not 100% sure about std::map vs QHash given the number of elements,
> this would need benchmarking.
>
> But I agree 100% that compiler-generated thread safety around a static is
> NOTHING compared to amount of nodes allocated to fill in a map or hash.
>
> @tcanabrava <https://phabricator.kde.org/p/tcanabrava/> Please have a
> look at https://gist.github.com/jboner/2841832
> Locking an available mutex is 4 times faster than even fetching data from
> main memory (i.e. data which isn't yet in a CPU cache). This is many orders
> of magnitude faster than creating a filling a map or a hash full of
> QStrings. On top of that, compilers don't even generate a full-blown mutex
> for threadsafe statics, they generate a three-state atomic (a bit like
> Q_GLOBAL_STATIC does) (3 because not created, being created, already
> created).
>
> The most performant solution is to turn the input string into a QByteArray
> and then perform a binary search in a C array of const char* (no
> allocations, even the very first time).
> The most readable (but still good, unlike the current code in git)
> solution is a static map.
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26126
>
> *To: *tcanabrava, ervin, dfaure
> *Cc: *dfaure, ervin, GB_2, apol, kde-frameworks-devel, LeGast00n,
> michaelh, ngraham, bruns
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191224/1c646656/attachment-0001.html>