dfaure requested changes to this revision.
dfaure added inline comments.
INLINE COMMENTS
> kconfig_compiler.cpp:1020
> + 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.
> 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 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
