pack-bitmaps: plug memory leak, fix allocation size for recent_bitmaps
PATCH 1/1 pack-bitmaps: plug memory leak, fix allocation size for recent_bitmaps
Use an automatic variable for recent_bitmaps, an array of pointers. This way we don't allocate too much and don't have to free the memory at the end. The old code over-allocated because it reserved enough memory to store all of the structs it is only pointing to and never freed it. 160 64-bit pointers take up 1280 bytes, which is not too much to be placed on the stack.
MAX_XOR_OFFSET is turned into a preprocessor constant to make it constant enough for use in an non-variable array declaration.
---
This seems to have fallen through the cracks, or did I just miss it?
pack-bitmap.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
pack-bitmap.c +3 -5
@@ -209,14 +209,12 @@ static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos)return buffer[(*pos)++]; } +#define MAX_XOR_OFFSET 160 +static int load_bitmap_entries_v1 (struct bitmap_index *index) { -static const size_t MAX_XOR_OFFSET =160 ; -uint32_t i; -struct stored_bitmap **recent_bitmaps; - - recent_bitmaps =xcalloc (MAX_XOR_OFFSET ,sizeof (struct stored_bitmap )); +struct stored_bitmap *recent_bitmaps[MAX_XOR_OFFSET ] = {NULL };for (i =0 ; i < index->entry_count; ++i) {int xor_offset, flags;
Signature · 1 line
2.4.1
Quoted 7 lines · René Scharfe
Use an automatic variable for
recent_bitmaps, an array of pointers. This way we don't allocate too much and don't have to free the memory at the end. The old code over-allocated because it reserved enough memory to store all of the structs it is only pointing to and never freed it. 160 64-bit pointers take up 1280 bytes, which is not too much to be placed on the stack.
Quoted 8 lines
MAX_XOR_OFFSETis turned into a preprocessor constant to make it constant enough for use in an non-variable array declaration.Noticed-by Stefan Beller <[email protected]>Suggested-by Jeff King <[email protected]>Signed-off-by Rene Scharfe <[email protected]>---
This seems to have fallen through the cracks, or did I just miss it?
Thanks, this looks good.
I looked over the function one more time to make sure it is the function that is wrong, and not my suggestion. :) The current code seems pretty obviously wrong.
-Peff
Quoted 18 lines · Jeff King
Quoted 7 lines · René Scharfe
Use an automatic variable for
recent_bitmaps, an array of pointers. This way we don't allocate too much and don't have to free the memory at the end. The old code over-allocated because it reserved enough memory to store all of the structs it is only pointing to and never freed it. 160 64-bit pointers take up 1280 bytes, which is not too much to be placed on the stack.Quoted 8 lines
MAX_XOR_OFFSETis turned into a preprocessor constant to make it constant enough for use in an non-variable array declaration.Noticed-by Stefan Beller <[email protected]>Suggested-by Jeff King <[email protected]>Signed-off-by Rene Scharfe <[email protected]>---
This seems to have fallen through the cracks, or did I just miss it?
Quoted 5 lines
Thanks, this looks good.
I looked over the function one more time to make sure it is the function that is wrong, and not my suggestion. :) The current code seems pretty obviously wrong.
I actually cannot guess what the current code is trying to do. Was it an attempt to cache that many entries, but instead allocated and discarded the space it tried to use as a cache every time?
Quoted 7 lines · Junio C Hamano
Quoted 3 lines
I looked over the function one more time to make sure it is the function that is wrong, and not my suggestion. :) The current code seems pretty obviously wrong.
I actually cannot guess what the current code is trying to do. Was it an attempt to cache that many entries, but instead allocated and discarded the space it tried to use as a cache every time?
Sort of. There are two caches at work.
The bitmaps on disk may be stored as XORs against nearby bitmaps up to MAX_XOR_OFFSET slots away. So our goal is to reconstruct the actual bitmaps and put them in our cache for later use. That's actually done by store_bitmap(), which puts them into a hash table indexed by sha1.
But because we get the XOR base as an offset, we also need to be able to quickly say "what was the bitmap that was N slots ago?", and the hash cannot answer that quickly. So we keep a sliding window of the last MAX_XOR_OFFSET bitmaps (pointing to the cached bitmaps stored in the hash), and then we can index that directly by offset. And we can reuse the array as a circular buffer (notice we always index it modulo the max offset).
So you can think of the recent_bitmaps as an auxiliary index into the bitmaps already cached by store_bitmap(). We don't need it after the xor reconstruction (actually I think we don't do the xor reconstruction here, but instead retain a pointer to the xor base and lazily do it, but the point is that we've created that pointer).
-Peff
Quoted 7 lines · René Scharfe
Use an automatic variable for
recent_bitmaps, an array of pointers. This way we don't allocate too much and don't have to free the memory at the end. The old code over-allocated because it reserved enough memory to store all of the structs it is only pointing to and never freed it. 160 64-bit pointers take up 1280 bytes, which is not too much to be placed on the stack.
Quoted 8 lines
MAX_XOR_OFFSETis turned into a preprocessor constant to make it constant enough for use in an non-variable array declaration.Noticed-by Stefan Beller <[email protected]>Suggested-by Jeff King <[email protected]>Signed-off-by Rene Scharfe <[email protected]>---
This seems to have fallen through the cracks, or did I just miss it?
I planned on resending it eventually. ;)
But with the usual definition of 'eventually',
it may take a while.
Thanks for picking this up!
Stefan
Quoted 9 lines
pack-bitmap.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)pack-bitmap.c +0
-0
pack-bitmap.c static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos) · line 214
Quoted 8 lines
pack-bitmap.c +2
-1@@ -209,14 +209,12 @@ static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos)return buffer[(*pos)++]; } +#define MAX_XOR_OFFSET 160 +static int load_bitmap_entries_v1 (struct bitmap_index *index) { -static const size_t MAX_XOR_OFFSET =160 ;
Is there a reason why we prefer defines over a static const size_t here?
pack-bitmap.c static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos) · line 217
Quoted 6 lines
pack-bitmap.c +1
-4-uint32_t i; -struct stored_bitmap **recent_bitmaps; - - recent_bitmaps =xcalloc (MAX_XOR_OFFSET ,sizeof (struct stored_bitmap )); +struct stored_bitmap *recent_bitmaps[MAX_XOR_OFFSET ] = {NULL };
This looks fine to me.
Quoted 10 lines
pack-bitmap.c +0
-1for (i =0 ; i < index->entry_count; ++i) {int xor_offset, flags; --2.4.1
--
To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 19.05.2015 um 02:57 schrieb Stefan Beller:
Quoted 9 lines · René Scharfe
Quoted 8 lines
pack-bitmap.c +0
-0@@ -209,14 +209,12 @@ static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos)return buffer[(*pos)++]; }
Quoted 7 lines
pack-bitmap.c static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos) · line 214
Quoted 5 lines
pack-bitmap.c +2
-1+#define MAX_XOR_OFFSET 160 +static int load_bitmap_entries_v1 (struct bitmap_index *index) { -static const size_t MAX_XOR_OFFSET =160 ;Is there a reason why we prefer defines over a static const
size_there?
Yes, see below.
Quoted 6 lines
pack-bitmap.c static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos) · line 217
Quoted 6 lines
pack-bitmap.c +1
-4-uint32_t i; -struct stored_bitmap **recent_bitmaps; - - recent_bitmaps =xcalloc (MAX_XOR_OFFSET ,sizeof (struct stored_bitmap )); +struct stored_bitmap *recent_bitmaps[MAX_XOR_OFFSET ] = {NULL };
If MAX_XOR_OFFSET is a const then C89 does not allow this declaration. C99 gives you a variable-length array here. VLAs can't be initialized at declaration time, so we'd need to add a memset() call (at least with GCC 4.9). Overall it's simpler and shorter to use a macro.
We could also use an enum, but that's not exactly common.
René