GNOME Bugzilla – Bug 704754
Port to libgee 0.8
Last modified: 2018-05-22 14:55:09 UTC
Currently Vala includes a bundled version of libgee. This have several drawbacks: - Currently the Vala compiler developers don't have access to such 'advanced' features as sorting or sorted collections. I saw at least several comments during writing this patch which are workarounds around missing features. - People using Vala compiler API (text editors etc.) have to either deal with 2, subtly different, collection libraries or use only the Vala bundled one missing the libgee features and improvements. - All the (speed etc.) improvements are included to the libgee 0.8. (For example in developers workflow (or even in normal build without lto) Vala compiler takes the most of compile time. It would be nice to parallelize it and Futures seems to be easier to use then raw threads) Possible drawbacks: - The gee needs to be installed before Vala. As both of them include .c files in distribution it does not seem to be a large problem even for people building from source.
Created attachment 249918 [details] [review] 0001-Port-to-gee-0.8.patch
Just as an information, parallelization inside valac is about to be out of question. It's way easier to use parvala (see in contrib/). I'd like to see some benchmarks to ensure that using libgee won't result in loss of performance.
(In reply to comment #2) > Just as an information, parallelization inside valac is about to be out of > question. It's way easier to use parvala (see in contrib/). Then use Lazy instead of comments that some value should not be called twice - the paralelization was just an example. The immediate improvements would be to just iterate over sorted set instead of 'recovering' the order from HashMap or sorting using built-in method instead of insertion sort implementation. > I'd like to see some benchmarks to ensure that using libgee won't result in > loss of performance. make check timings (test done once) original: 10.51s user 2.23s system 93% cpu 13.600 total with patch (0.10): 11.23s user 2.19s system 94% cpu 14.262 total with patch (master): 11.18s user 2.15s system 94% cpu 14.163 total with port to TreeMap: 11.37s user 2.13s system 91% cpu 14.713 total with port to .sort: 11.46s user 1.93s system 93% cpu 14.388 total The TreeMap patch would require building from git to get the fix for foreach over values). I haven't check the .foreach in all places as the patch is huge and probably solving the bug #686392 in one way or another is better way. So it (straightforward port) comes with about 5% overhead at this moment with some space to be decreased (by improvements in Vala compiler or libgee).
(In reply to comment #3) > (In reply to comment #2) > > Just as an information, parallelization inside valac is about to be out of > > question. It's way easier to use parvala (see in contrib/). > > Then use Lazy instead of comments that some value should not be called twice - > the paralelization was just an example. The immediate improvements would be to > just iterate over sorted set instead of 'recovering' the order from HashMap or > sorting using built-in method instead of insertion sort implementation. > > > I'd like to see some benchmarks to ensure that using libgee won't result in > > loss of performance. > > make check timings (test done once) > > original: 10.51s user 2.23s system 93% cpu 13.600 total > with patch (0.10): 11.23s user 2.19s system 94% cpu 14.262 total > with patch (master): 11.18s user 2.15s system 94% cpu 14.163 total > with port to TreeMap: 11.37s user 2.13s system 91% cpu 14.713 total > with port to .sort: 11.46s user 1.93s system 93% cpu 14.388 total > with partial port to .foreach (0.10): 10.72s user 1.66s system 92% cpu 13.429 total with partial port to .foreach (master): make check 10.94s user 1.99s system 93% cpu 13.762 total Port contained only: - Parts which did not caused bug #706243 to trigger - only vala/*.vala - only foreach which would produce 'nice' code. The further improvements of the port would be expected once bug #704420 is done and/or once improvements discussed in bug #645850 are done.
Created attachment 252112 [details] [review] 0001-Port-to-gee-0.8.patch Port rebased to master
Created attachment 252113 [details] [review] 0002-Port-to-SortedMap.patch
Created attachment 252114 [details] [review] 0003-Sort-the-symbols-using-.sort.patch
Created attachment 252115 [details] [review] 0004-Move-to-.foreach-methods-where-it-does-not-impact-re.patch
Created attachment 252871 [details] [review] Port to gee-0.8 In my current tests speed of the master vs. those patches are: - Master: 8.23 (user) 9.544 (total) - patches + gee 0.10: 8.98 (user) 10.344 (total) - patches + gee 0.11: 8.86 (user) 10.195 (total) I guess it needs to wait for fix of bug #704420
Created attachment 252872 [details] [review] Port to SortedMap
Created attachment 252873 [details] [review] Sort the symbols using .sort
Created attachment 252874 [details] [review] Move to .foreach methods where it does not impact readability
Hey, please, don't. It breaks valadocs driver infrastructure.
(In reply to comment #13) > Hey, > > please, don't. It breaks valadocs driver infrastructure. To sum up the discussion on IRC. The problem will is that valadoc uses libgee internally. For some reasons it have multiple drivers instead of a suffixed binary (as valac). It is documented somewhere. My personal note: For sure the change is nothing that will happen quickly so there is time to think about it calmly. It would be nice to know the reason for dynamic loading the drivers (it's documented somewhere but Florian don't remember where). If there is a good reason why user program might want to load all possible libvalas and we assume that libgee API/ABI might break in foreseeable future it makes sense to just copy libgee into vala. I cannot see what the reason might be and I still believe that point 2 stands and possible problems of valadoc might be solved by building it for specific valac or ranges of valac - however it depends on the reason why drivers have been introduced. In any case it's better to solve the valadoc problem before making the change (and change the scope of change accordingly).
Rebased and updated with the need for hard testing. https://git.gnome.org/browse/vala/log/?h=wip/gee
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/398.