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 <nore...@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 >
