Conversation
4bee801 to
bc3dd7b
Compare
|
I've started the conversion to get an estimate on how it influences the code. Currently, with 2/5 maps converted, we save around 2% in size for the Before: After: Guess it'll amount to around 5% in the end. No performance measurements yet. Will do something more involved to see if this actually imposes a real performance hit for us. But most importantly, it looks a lot nicer and is easier to grok. See https://github.com/pks-t/libgit2/tree/pks/map-macros-to-functions. |
|
In contrast to what I've said initially, I've now also included the actual conversion from macros to functions in addition to the cleanups. There wasn't much gained in terms of resulting code size by the additional two maps being converted to actual functions. Guess this results from the fact that they were both just used in one file - so actually it stays around 2% decreased code size. I've done some small performance tests. The conversion at least has no impact on our testing suite in terms of performance, which is good. I also did some tests benchmarking the maps directly by calling map-modifying functions over 10 million iterations. There was no performance impact, as well. While these numbers are not really representative for real-world usage, the at least show that there's no immediate performance penalty by not having them inlined anymore. And actually, I think the result is much easier to work with and reason about than what we previously had. |
|
By the way, the failures are the usual threading/network related issues |
|
I'm really happy with this; I think that this is a really big improvement. If I could pick just one nit: we use
(I realize this is quite nitpicky; sorry.) |
6b15959 to
8f1ff26
Compare
This is a crash on 32-bit and I assume that it doesn't do the right thing on 64-bit either. MSVC emits a warning for this, but of course, it's easy to get lost among all of the similar 'possible loss of data' warnings.
Fixes a regression from libgit2#4092. This is a crash on 32-bit and I assume that it doesn't do the right thing on 64-bit either. MSVC emits a warning for this, but of course, it's easy to get lost among all of the similar 'possible loss of data' warnings.
So this is a first batch of cleanups for khash-related code. It mainly only transforms all calls to
kh_functions to instead use the type-specific functions (e.g.git_strmap_putinstead ofkh_put). As is, this does not really change anything in behavior, but it improves encapsulation here.So my goal later on is to convert the types to use real functions instead. I'm not yet sure if this goal is worthwhile after all and how it would impact performance. But the benefit is two-fold:
We do not have to expose all these externals regarding the macro-magic, for example the nasty
GIT__USE_STRMAPthing and others. Instead, I want to include it inside of the respective implementation file, e.g.src/strmap.cand be done with it. This might even reduce the resulting binary size (if the linker didn't fold the different implementations together).It gets rid of those "magic" macros which looked like normal function calls, but did not behave as such, which were initially bothering me. As ever so often, when one starts to fix up a small annoyance one ends up walking a rather long path, trying to fix everything as you go. But, well. Why not.
I'd appreciate opinions on whether you think the overall goal is worthwhile or not. Disregarding of whether the overall goal (that is creating real functions) is worthwhile, I guess this PR is still worth being merged.