On Sat, Jul 21, 2018 at 06:23:32AM +0200, Duy Nguyen wrote: > > I'm not sure I completely agree with this. While 4GB deltas should be > > pretty rare, the nice thing about 64-bit is that you never have to even > > think about whether the variable is large enough. I think the 2^32 > > limitations on Windows are something we should be fixing in the long > > term (though there it is even worse because it is not just deltas, but > > entire objects). > > I guess that means we stick to uint64_t then? It does increase more > memory on 32-bit architecture (and probably processing cost as well > because 64-bit uses up 2 registers).
Yes, but if we switch away from an array to a hash, then we get the best of both worlds: we are using 64-bits to store the size, but we only need an entry for deltas that are actually big. Then the common small-delta case remains efficient in both CPU and memory, and we pay the costs only in proportion to the number of large deltas (though the hash is a slightly higher cost for those large deltas than an array). > > This is new in this iteration. I guess this is to cover the case where > > we are built with pthread support, but --threads=1? > > If you mean the "lock_initialized" variable, not really. the other > _lock() macros in builtin/ call pthread_mutex_lock() unconditionally, > which is fine. But I feel a bit uncomfortable doing the same in > pack-objects.h which technically is library code (but yes practically > is a long arm of builtin/pack-objects.c), so lock_initialized is there > to make sure we don't touch uninitialized locks if somebody forgets to > init them first. I think the ones in builtin/ check threads_active to avoid actually locking. And that's set in init_thread(), which we do not call if we are using a single thread. So I think this is basically doing the same thing, but with a separate flag (since the library code does not know about threads_active). > > Your original patch had to copy the oe_* helpers into the file to handle > > that. But I think we're essentially just locking the whole functions: > > I'll try to avoid this lock when deltas are small and see if it helps > the linux.git case on Elijah's machine. So we may end up locking just > a part of these functions. Yeah, I think my suggestion doesn't work once we start doing more complex locking logic. Let's just forget it. I think the "lock_initialized" thing is probably the right approach. It might be worth getting rid of builtin/pack-objects.c's local threads_active variable, and just using to_pack.threads_active. The two flag would always want to match. -Peff
