{"thread":{"id":"58183","subject":"[PATCH] midx: reduce memory pressure while writing bitmaps","startedAt":"2022-07-18T20:36:12Z","lastAt":"2022-07-19T15:59:36Z","messageCount":8,"participants":["Derrick Stolee via GitGitGadget","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"459329","messageId":"pull.1292.git.1658176565751.gitgitgadget@gmail.com","threadId":"58183","inReplyTo":null,"subject":"[PATCH] midx: reduce memory pressure while writing bitmaps","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-18T20:36:05Z","receivedAt":"2022-07-18T20:36:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWe noticed that some 'git multi-pack-index write --bitmap' processes\nwere running with very high memory. It turns out that a lot of this\nmemory is required to store a list of every object in the written\nmulti-pack-index, with a second copy that has additional information\nused for the bitmap writing logic.\n\nUsing 'valgrind --tool=massif' before this change, the following chart\nshows how memory load increased and was maintained throughout the\nprocess:\n\n    GB\n4.102^                                                             ::\n     |              @  @::@@::@@::::::::@::::::@@:#:::::::::::::@@:: :\n     |         :::::@@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |      :::: :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |    :::: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |    : :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |    : :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @ :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @ :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n   0 +--------------------------------------------------------------->\n\nIt turns out that the 'struct write_midx_context' data is persisting\nthrough the life of the process, including the 'entries' array. This\narray is used last inside find_commits_for_midx_bitmap() within\nwrite_midx_bitmap(). If we free (and nullify) the array at that point,\nwe can free a decent chunk of memory before the bitmap logic adds more\nto the memory footprint.\n\nHere is the massif memory load chart after this change:\n\n    GB\n3.111^      #\n     |      #                              :::::::::::@::::::::::::::@\n     |      #        ::::::::::::::::::::::::: : :: : @:: ::::: :: ::@\n     |     @#  :::::::::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :::@#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n   0 +--------------------------------------------------------------->\n\nIt is unfortunate that the lifetime of the 'entries' array is less\nclear. To make this simpler, I added a few things to try and prevent an\naccidental reference:\n\n 1. Using FREE_AND_NULL() we will at least get a segfault from reading a\n    NULL pointer instead of a use-after-free.\n\n 2. 'entries_nr' is also set to zero to make any loop that would iterate\n    over the entries be trivial.\n\n 3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does\n    not get another reference later. This requires adding a local copy\n    of 'pack_order' giving us a reference that we can use later in the\n    method.\n\n 4. Add significant comments in write_midx_bitmap() and\n    write_midx_internal() to add warnings for future authors who might\n    accidentally add references to this cleared memory.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    midx: reduce memory pressure while writing bitmaps\n    \n    The thing I'm most worried about with this patch is whether there is\n    enough (or too much) defensive programming.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1292%2Fderrickstolee%2Fbitmap-memory-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1292/derrickstolee/bitmap-memory-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1292\n\n midx.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 5f0dd386b02..cc31d803a5f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1063,6 +1063,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \tstruct commit **commits = NULL;\n \tuint32_t i, commits_nr;\n \tuint16_t options = 0;\n+\tuint32_t *pack_order;\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n \tint ret;\n \n@@ -1076,6 +1077,15 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \n \tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);\n \n+\t/*\n+\t * Remove the ctx.entries to reduce memory pressure.\n+\t * Nullify 'ctx' to help avoid adding new references to ctx->entries.\n+\t */\n+\tFREE_AND_NULL(ctx->entries);\n+\tctx->entries_nr = 0;\n+\tpack_order = ctx->pack_order;\n+\tctx = NULL;\n+\n \t/*\n \t * Build the MIDX-order index based on pdata.objects (which is already\n \t * in MIDX order; c.f., 'midx_pack_order_cmp()' for the definition of\n@@ -1102,7 +1112,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \t * bitmap_writer_finish().\n \t */\n \tfor (i = 0; i < pdata.nr_objects; i++)\n-\t\tindex[ctx->pack_order[i]] = &pdata.objects[i].idx;\n+\t\tindex[pack_order[i]] = &pdata.objects[i].idx;\n \n \tbitmap_writer_select_commits(commits, commits_nr, -1);\n \tret = bitmap_writer_build(&pdata);\n@@ -1443,6 +1453,11 @@ static int write_midx_internal(const char *object_dir,\n \tif (flags & MIDX_WRITE_REV_INDEX &&\n \t    git_env_bool(\"GIT_TEST_MIDX_WRITE_REV\", 0))\n \t\twrite_midx_reverse_index(midx_name.buf, midx_hash, &ctx);\n+\n+\t/*\n+\t * Writing the bitmap must be last, as it will free ctx.entries\n+\t * to reduce memory pressure during the bitmap write.\n+\t */\n \tif (flags & MIDX_WRITE_BITMAP) {\n \t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,\n \t\t\t\t      refs_snapshot, flags) < 0) {\n\nbase-commit: 9dd64cb4d310986dd7b8ca7fff92f9b61e0bd21a\n-- \ngitgitgadget\n"},{"id":"459336","messageId":"220718.867d4aw0k3.gmgdl@evledraar.gmail.com","threadId":"58183","inReplyTo":"pull.1292.git.1658176565751.gitgitgadget@gmail.com","subject":"Re: [PATCH] midx: reduce memory pressure while writing bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-18T21:47:12Z","receivedAt":"2022-07-18T21:52:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 18 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n> [...]\n> It is unfortunate that the lifetime of the 'entries' array is less\n> clear. To make this simpler, I added a few things to try and prevent an\n> accidental reference:\n>\n>  1. Using FREE_AND_NULL() we will at least get a segfault from reading a\n>     NULL pointer instead of a use-after-free.\n>\n>  2. 'entries_nr' is also set to zero to make any loop that would iterate\n>     over the entries be trivial.\n>\n>  3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does\n>     not get another reference later. This requires adding a local copy\n>     of 'pack_order' giving us a reference that we can use later in the\n>     method.\n>\n>  4. Add significant comments in write_midx_bitmap() and\n>     write_midx_internal() to add warnings for future authors who might\n>     accidentally add references to this cleared memory.\n> [...]\n> +\t/*\n> +\t * Remove the ctx.entries to reduce memory pressure.\n> +\t * Nullify 'ctx' to help avoid adding new references to ctx->entries.\n> +\t */\n> +\tFREE_AND_NULL(ctx->entries);\n> +\tctx->entries_nr = 0;\n> +\tpack_order = ctx->pack_order;\n> +\tctx = NULL;\n\nAfter this change this is a ~70 line function, but only 3 lines at the\ntop actually use ctx for anything:\n    \n\t/* the bug check for ctx.nr... */\n\tprepare_midx_packing_data(&pdata, ctx);\n\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);\n\nDid you consider just splitting it up so that that there's a \"prepare\nwrite\" function? Then you don't need to worry about the scoping of ctx.\n\nI'd think that would be better, then you also wouldn't need to implement\nyour own free-ing, nothing after this seems to use ctx->entries_nr (but\nI just skimmed it), so it could just fall through to the free() at the\nend of write_midx_internal() (the only caller), couldn't it?\n"},{"id":"459366","messageId":"335a5d52-ea9b-565b-396d-6543451aa027@github.com","threadId":"58183","inReplyTo":"220718.867d4aw0k3.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] midx: reduce memory pressure while writing bitmaps","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-19T13:50:51Z","receivedAt":"2022-07-19T14:18:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/18/2022 5:47 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Jul 18 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee <derrickstolee@github.com>\n>> [...]\n>> It is unfortunate that the lifetime of the 'entries' array is less\n>> clear. To make this simpler, I added a few things to try and prevent an\n>> accidental reference:\n>>\n>>  1. Using FREE_AND_NULL() we will at least get a segfault from reading a\n>>     NULL pointer instead of a use-after-free.\n>>\n>>  2. 'entries_nr' is also set to zero to make any loop that would iterate\n>>     over the entries be trivial.\n>>\n>>  3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does\n>>     not get another reference later. This requires adding a local copy\n>>     of 'pack_order' giving us a reference that we can use later in the\n>>     method.\n>>\n>>  4. Add significant comments in write_midx_bitmap() and\n>>     write_midx_internal() to add warnings for future authors who might\n>>     accidentally add references to this cleared memory.\n>> [...]\n>> +\t/*\n>> +\t * Remove the ctx.entries to reduce memory pressure.\n>> +\t * Nullify 'ctx' to help avoid adding new references to ctx->entries.\n>> +\t */\n>> +\tFREE_AND_NULL(ctx->entries);\n>> +\tctx->entries_nr = 0;\n>> +\tpack_order = ctx->pack_order;\n>> +\tctx = NULL;\n> \n> After this change this is a ~70 line function, but only 3 lines at the\n> top actually use ctx for anything:\n>     \n> \t/* the bug check for ctx.nr... */\n> \tprepare_midx_packing_data(&pdata, ctx);\n> \tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);\n> \n> Did you consider just splitting it up so that that there's a \"prepare\n> write\" function? Then you don't need to worry about the scoping of ctx.\n\nI did not, and that's a good suggestion. Extracting these prepare steps\ninto write_midx_internal() works to reduce the complexity and make the\nearly free()ing more clear.\n \n> I'd think that would be better, then you also wouldn't need to implement\n> your own free-ing, nothing after this seems to use ctx->entries_nr (but\n> I just skimmed it), so it could just fall through to the free() at the\n> end of write_midx_internal() (the only caller), couldn't it?\n\nI think this paragraph misunderstands the point. The bitmaps are being\ncomputed and written before the MIDX lock file completes, so the free()\nof the entries array is after the bitmaps are computed. To reduce the\nmemory pressure (by ~25%) by freeing early is the point of this patch.\n\nWe still want that free(ctx.entries) after the cleanup: label for the\nerror cases, but for the \"happy path\" we can free early.\n\nBy doing the refactoring, this point of having an earlier free() makes\nthings more clear.\n\nThanks,\n-Stolee\n"},{"id":"459370","messageId":"pull.1292.v2.git.1658244366.gitgitgadget@gmail.com","threadId":"58183","inReplyTo":"pull.1292.git.1658176565751.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] midx: reduce memory pressure while writing bitmaps","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-19T15:26:03Z","receivedAt":"2022-07-19T15:26:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"We noticed an instance where writing multi-pack-index bitmaps was taking a\nlot of memory. This small change can reduce the memory pressure slightly\n(~25%), but more will be needed to significantly reduce the memory pressure.\nSuch a change would require updating the bitmap writing code to use on-disk\ndata structures instead. This is particularly tricky when the\nmulti-pack-index has not been fully written, because we don't want a point\nin time where the object store has a new multi-pack-index without a\nreachability bitmap.\n\n\nUpdates in v2\n=============\n\nTo reduce confusion on the lifetime of 'ctx.entries', some refactoring\npatches are inserted to first extract the use of 'ctx' out of\nwrite_midx_bitmap() and into write_midx_internal(). This makes the\nFREE_AND_NULL() stand out more clearly.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n  pack-bitmap-write: use const for hashes\n  midx: extract bitmap write setup\n  midx: reduce memory pressure while writing bitmaps\n\n midx.c              | 69 +++++++++++++++++++++++++++++----------------\n pack-bitmap-write.c |  2 +-\n pack-bitmap.h       |  2 +-\n 3 files changed, 47 insertions(+), 26 deletions(-)\n\n\nbase-commit: 9dd64cb4d310986dd7b8ca7fff92f9b61e0bd21a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1292%2Fderrickstolee%2Fbitmap-memory-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1292/derrickstolee/bitmap-memory-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1292\n\nRange-diff vs v1:\n\n -:  ----------- > 1:  a09fdbb8f3e pack-bitmap-write: use const for hashes\n 1:  9104bc55795 ! 2:  4dfb7ae5112 midx:  reduce memory pressure while writing bitmaps\n     @@ Metadata\n      Author: Derrick Stolee <derrickstolee@github.com>\n      \n       ## Commit message ##\n     -    midx:  reduce memory pressure while writing bitmaps\n     +    midx: extract bitmap write setup\n      \n     -    We noticed that some 'git multi-pack-index write --bitmap' processes\n     -    were running with very high memory. It turns out that a lot of this\n     -    memory is required to store a list of every object in the written\n     -    multi-pack-index, with a second copy that has additional information\n     -    used for the bitmap writing logic.\n     +    The write_midx_bitmap() method is a long method that does a lot of\n     +    steps. It requires the write_midx_context struct for use in\n     +    prepare_midx_packing_data() and find_commits_for_midx_bitmap(), but\n     +    after that only needs the pack_order array.\n      \n     -    Using 'valgrind --tool=massif' before this change, the following chart\n     -    shows how memory load increased and was maintained throughout the\n     -    process:\n     -\n     -        GB\n     -    4.102^                                                             ::\n     -         |              @  @::@@::@@::::::::@::::::@@:#:::::::::::::@@:: :\n     -         |         :::::@@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |      :::: :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |    :::: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |    : :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |    : :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         | @ :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         | @ :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -         | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     -       0 +--------------------------------------------------------------->\n     -\n     -    It turns out that the 'struct write_midx_context' data is persisting\n     -    through the life of the process, including the 'entries' array. This\n     -    array is used last inside find_commits_for_midx_bitmap() within\n     -    write_midx_bitmap(). If we free (and nullify) the array at that point,\n     -    we can free a decent chunk of memory before the bitmap logic adds more\n     -    to the memory footprint.\n     -\n     -    Here is the massif memory load chart after this change:\n     -\n     -        GB\n     -    3.111^      #\n     -         |      #                              :::::::::::@::::::::::::::@\n     -         |      #        ::::::::::::::::::::::::: : :: : @:: ::::: :: ::@\n     -         |     @#  :::::::::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |  :::@#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -         |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     -       0 +--------------------------------------------------------------->\n     -\n     -    It is unfortunate that the lifetime of the 'entries' array is less\n     -    clear. To make this simpler, I added a few things to try and prevent an\n     -    accidental reference:\n     -\n     -     1. Using FREE_AND_NULL() we will at least get a segfault from reading a\n     -        NULL pointer instead of a use-after-free.\n     -\n     -     2. 'entries_nr' is also set to zero to make any loop that would iterate\n     -        over the entries be trivial.\n     -\n     -     3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does\n     -        not get another reference later. This requires adding a local copy\n     -        of 'pack_order' giving us a reference that we can use later in the\n     -        method.\n     -\n     -     4. Add significant comments in write_midx_bitmap() and\n     -        write_midx_internal() to add warnings for future authors who might\n     -        accidentally add references to this cleared memory.\n     +    This is a messy, but completely non-functional refactoring. The code is\n     +    only being moved around to reduce visibility of the write_midx_context\n     +    during the longest part of computing reachability bitmaps.\n      \n          Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n      \n       ## midx.c ##\n     -@@ midx.c: static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n     - \tstruct commit **commits = NULL;\n     - \tuint32_t i, commits_nr;\n     - \tuint16_t options = 0;\n     -+\tuint32_t *pack_order;\n     - \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n     - \tint ret;\n     +@@ midx.c: static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr\n     + \treturn cb.commits;\n     + }\n       \n     -@@ midx.c: static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n     +-static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n     +-\t\t\t     struct write_midx_context *ctx,\n     ++static int write_midx_bitmap(const char *midx_name,\n     ++\t\t\t     const unsigned char *midx_hash,\n     ++\t\t\t     struct packing_data *pdata,\n     ++\t\t\t     struct commit **commits,\n     ++\t\t\t     uint32_t commits_nr,\n     ++\t\t\t     uint32_t *pack_order,\n     + \t\t\t     const char *refs_snapshot,\n     + \t\t\t     unsigned flags)\n     + {\n     +-\tstruct packing_data pdata;\n     +-\tstruct pack_idx_entry **index;\n     +-\tstruct commit **commits = NULL;\n     +-\tuint32_t i, commits_nr;\n     ++\tint ret, i;\n     + \tuint16_t options = 0;\n     +-\tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n     +-\tint ret;\n     +-\n     +-\tif (!ctx->entries_nr)\n     +-\t\tBUG(\"cannot write a bitmap without any objects\");\n     ++\tstruct pack_idx_entry **index;\n     ++\tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name,\n     ++\t\t\t\t\thash_to_hex(midx_hash));\n       \n     - \tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);\n     + \tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n     + \t\toptions |= BITMAP_OPT_HASH_CACHE;\n       \n     -+\t/*\n     -+\t * Remove the ctx.entries to reduce memory pressure.\n     -+\t * Nullify 'ctx' to help avoid adding new references to ctx->entries.\n     -+\t */\n     -+\tFREE_AND_NULL(ctx->entries);\n     -+\tctx->entries_nr = 0;\n     -+\tpack_order = ctx->pack_order;\n     -+\tctx = NULL;\n     -+\n     +-\tprepare_midx_packing_data(&pdata, ctx);\n     +-\n     +-\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);\n     +-\n       \t/*\n       \t * Build the MIDX-order index based on pdata.objects (which is already\n       \t * in MIDX order; c.f., 'midx_pack_order_cmp()' for the definition of\n     + \t * this order).\n     + \t */\n     +-\tALLOC_ARRAY(index, pdata.nr_objects);\n     +-\tfor (i = 0; i < pdata.nr_objects; i++)\n     +-\t\tindex[i] = &pdata.objects[i].idx;\n     ++\tALLOC_ARRAY(index, pdata->nr_objects);\n     ++\tfor (i = 0; i < pdata->nr_objects; i++)\n     ++\t\tindex[i] = &pdata->objects[i].idx;\n     + \n     + \tbitmap_writer_show_progress(flags & MIDX_PROGRESS);\n     +-\tbitmap_writer_build_type_index(&pdata, index, pdata.nr_objects);\n     ++\tbitmap_writer_build_type_index(pdata, index, pdata->nr_objects);\n     + \n     + \t/*\n     + \t * bitmap_writer_finish expects objects in lex order, but pack_order\n      @@ midx.c: static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n     + \t * happens between bitmap_writer_build_type_index() and\n       \t * bitmap_writer_finish().\n       \t */\n     - \tfor (i = 0; i < pdata.nr_objects; i++)\n     +-\tfor (i = 0; i < pdata.nr_objects; i++)\n      -\t\tindex[ctx->pack_order[i]] = &pdata.objects[i].idx;\n     -+\t\tindex[pack_order[i]] = &pdata.objects[i].idx;\n     ++\tfor (i = 0; i < pdata->nr_objects; i++)\n     ++\t\tindex[pack_order[i]] = &pdata->objects[i].idx;\n       \n       \tbitmap_writer_select_commits(commits, commits_nr, -1);\n     - \tret = bitmap_writer_build(&pdata);\n     +-\tret = bitmap_writer_build(&pdata);\n     ++\tret = bitmap_writer_build(pdata);\n     + \tif (ret < 0)\n     + \t\tgoto cleanup;\n     + \n     + \tbitmap_writer_set_checksum(midx_hash);\n     +-\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, options);\n     ++\tbitmap_writer_finish(index, pdata->nr_objects, bitmap_name, options);\n     + \n     + cleanup:\n     + \tfree(index);\n      @@ midx.c: static int write_midx_internal(const char *object_dir,\n       \tif (flags & MIDX_WRITE_REV_INDEX &&\n       \t    git_env_bool(\"GIT_TEST_MIDX_WRITE_REV\", 0))\n       \t\twrite_midx_reverse_index(midx_name.buf, midx_hash, &ctx);\n      +\n     -+\t/*\n     -+\t * Writing the bitmap must be last, as it will free ctx.entries\n     -+\t * to reduce memory pressure during the bitmap write.\n     -+\t */\n       \tif (flags & MIDX_WRITE_BITMAP) {\n     - \t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,\n     +-\t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,\n     ++\t\tstruct packing_data pdata;\n     ++\t\tstruct commit **commits;\n     ++\t\tuint32_t commits_nr;\n     ++\n     ++\t\tif (!ctx.entries_nr)\n     ++\t\t\tBUG(\"cannot write a bitmap without any objects\");\n     ++\n     ++\t\tprepare_midx_packing_data(&pdata, &ctx);\n     ++\n     ++\t\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, &ctx);\n     ++\n     ++\t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &pdata,\n     ++\t\t\t\t      commits, commits_nr, ctx.pack_order,\n       \t\t\t\t      refs_snapshot, flags) < 0) {\n     + \t\t\terror(_(\"could not write multi-pack bitmap\"));\n     + \t\t\tresult = 1;\n -:  ----------- > 3:  98e72f71b6b midx: reduce memory pressure while writing bitmaps\n\n-- \ngitgitgadget\n"},{"id":"459371","messageId":"a09fdbb8f3e42ff657cc6a0e66fde420afbe2af1.1658244366.git.gitgitgadget@gmail.com","threadId":"58183","inReplyTo":"pull.1292.v2.git.1658244366.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] pack-bitmap-write: use const for hashes","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-19T15:26:04Z","receivedAt":"2022-07-19T15:26:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThe next change will use a const array when calling this method. There\nis no need for the non-const version, so let's do this cleanup quickly.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n pack-bitmap-write.c | 2 +-\n pack-bitmap.h       | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex c43375bd344..4fcfaed428f 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -683,7 +683,7 @@ static void write_hash_cache(struct hashfile *f,\n \t}\n }\n \n-void bitmap_writer_set_checksum(unsigned char *sha1)\n+void bitmap_writer_set_checksum(const unsigned char *sha1)\n {\n \thashcpy(writer.pack_checksum, sha1);\n }\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 3d3ddd77345..f3a57ca065f 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -75,7 +75,7 @@ int bitmap_has_oid_in_uninteresting(struct bitmap_index *, const struct object_i\n off_t get_disk_usage_from_bitmap(struct bitmap_index *, struct rev_info *);\n \n void bitmap_writer_show_progress(int show);\n-void bitmap_writer_set_checksum(unsigned char *sha1);\n+void bitmap_writer_set_checksum(const unsigned char *sha1);\n void bitmap_writer_build_type_index(struct packing_data *to_pack,\n \t\t\t\t    struct pack_idx_entry **index,\n \t\t\t\t    uint32_t index_nr);\n-- \ngitgitgadget\n\n"},{"id":"459372","messageId":"4dfb7ae5112ebf69de779d59522477cfe6f3b210.1658244366.git.gitgitgadget@gmail.com","threadId":"58183","inReplyTo":"pull.1292.v2.git.1658244366.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] midx: extract bitmap write setup","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-19T15:26:05Z","receivedAt":"2022-07-19T15:26:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThe write_midx_bitmap() method is a long method that does a lot of\nsteps. It requires the write_midx_context struct for use in\nprepare_midx_packing_data() and find_commits_for_midx_bitmap(), but\nafter that only needs the pack_order array.\n\nThis is a messy, but completely non-functional refactoring. The code is\nonly being moved around to reduce visibility of the write_midx_context\nduring the longest part of computing reachability bitmaps.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n midx.c | 56 ++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 32 insertions(+), 24 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 5f0dd386b02..e2dd808b35d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1053,40 +1053,35 @@ static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr\n \treturn cb.commits;\n }\n \n-static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n-\t\t\t     struct write_midx_context *ctx,\n+static int write_midx_bitmap(const char *midx_name,\n+\t\t\t     const unsigned char *midx_hash,\n+\t\t\t     struct packing_data *pdata,\n+\t\t\t     struct commit **commits,\n+\t\t\t     uint32_t commits_nr,\n+\t\t\t     uint32_t *pack_order,\n \t\t\t     const char *refs_snapshot,\n \t\t\t     unsigned flags)\n {\n-\tstruct packing_data pdata;\n-\tstruct pack_idx_entry **index;\n-\tstruct commit **commits = NULL;\n-\tuint32_t i, commits_nr;\n+\tint ret, i;\n \tuint16_t options = 0;\n-\tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n-\tint ret;\n-\n-\tif (!ctx->entries_nr)\n-\t\tBUG(\"cannot write a bitmap without any objects\");\n+\tstruct pack_idx_entry **index;\n+\tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name,\n+\t\t\t\t\thash_to_hex(midx_hash));\n \n \tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n \t\toptions |= BITMAP_OPT_HASH_CACHE;\n \n-\tprepare_midx_packing_data(&pdata, ctx);\n-\n-\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);\n-\n \t/*\n \t * Build the MIDX-order index based on pdata.objects (which is already\n \t * in MIDX order; c.f., 'midx_pack_order_cmp()' for the definition of\n \t * this order).\n \t */\n-\tALLOC_ARRAY(index, pdata.nr_objects);\n-\tfor (i = 0; i < pdata.nr_objects; i++)\n-\t\tindex[i] = &pdata.objects[i].idx;\n+\tALLOC_ARRAY(index, pdata->nr_objects);\n+\tfor (i = 0; i < pdata->nr_objects; i++)\n+\t\tindex[i] = &pdata->objects[i].idx;\n \n \tbitmap_writer_show_progress(flags & MIDX_PROGRESS);\n-\tbitmap_writer_build_type_index(&pdata, index, pdata.nr_objects);\n+\tbitmap_writer_build_type_index(pdata, index, pdata->nr_objects);\n \n \t/*\n \t * bitmap_writer_finish expects objects in lex order, but pack_order\n@@ -1101,16 +1096,16 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \t * happens between bitmap_writer_build_type_index() and\n \t * bitmap_writer_finish().\n \t */\n-\tfor (i = 0; i < pdata.nr_objects; i++)\n-\t\tindex[ctx->pack_order[i]] = &pdata.objects[i].idx;\n+\tfor (i = 0; i < pdata->nr_objects; i++)\n+\t\tindex[pack_order[i]] = &pdata->objects[i].idx;\n \n \tbitmap_writer_select_commits(commits, commits_nr, -1);\n-\tret = bitmap_writer_build(&pdata);\n+\tret = bitmap_writer_build(pdata);\n \tif (ret < 0)\n \t\tgoto cleanup;\n \n \tbitmap_writer_set_checksum(midx_hash);\n-\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, options);\n+\tbitmap_writer_finish(index, pdata->nr_objects, bitmap_name, options);\n \n cleanup:\n \tfree(index);\n@@ -1443,8 +1438,21 @@ static int write_midx_internal(const char *object_dir,\n \tif (flags & MIDX_WRITE_REV_INDEX &&\n \t    git_env_bool(\"GIT_TEST_MIDX_WRITE_REV\", 0))\n \t\twrite_midx_reverse_index(midx_name.buf, midx_hash, &ctx);\n+\n \tif (flags & MIDX_WRITE_BITMAP) {\n-\t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,\n+\t\tstruct packing_data pdata;\n+\t\tstruct commit **commits;\n+\t\tuint32_t commits_nr;\n+\n+\t\tif (!ctx.entries_nr)\n+\t\t\tBUG(\"cannot write a bitmap without any objects\");\n+\n+\t\tprepare_midx_packing_data(&pdata, &ctx);\n+\n+\t\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, &ctx);\n+\n+\t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &pdata,\n+\t\t\t\t      commits, commits_nr, ctx.pack_order,\n \t\t\t\t      refs_snapshot, flags) < 0) {\n \t\t\terror(_(\"could not write multi-pack bitmap\"));\n \t\t\tresult = 1;\n-- \ngitgitgadget\n\n"},{"id":"459373","messageId":"98e72f71b6bec6f5c2df4139ca3df37d97ddcf54.1658244366.git.gitgitgadget@gmail.com","threadId":"58183","inReplyTo":"pull.1292.v2.git.1658244366.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] midx: reduce memory pressure while writing bitmaps","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-19T15:26:06Z","receivedAt":"2022-07-19T15:26:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWe noticed that some 'git multi-pack-index write --bitmap' processes\nwere running with very high memory. It turns out that a lot of this\nmemory is required to store a list of every object in the written\nmulti-pack-index, with a second copy that has additional information\nused for the bitmap writing logic.\n\nUsing 'valgrind --tool=massif' before this change, the following chart\nshows how memory load increased and was maintained throughout the\nprocess:\n\n    GB\n4.102^                                                             ::\n     |              @  @::@@::@@::::::::@::::::@@:#:::::::::::::@@:: :\n     |         :::::@@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |      :::: :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |    :::: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |    : :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |    : :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     |   :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @ :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @ :: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n     | @::: :: : :: @@:@: @ ::@ ::: ::::@: ::: @@:#:::::: :: : :@ :: :\n   0 +--------------------------------------------------------------->\n\nIt turns out that the 'struct write_midx_context' data is persisting\nthrough the life of the process, including the 'entries' array. This\narray is used last inside find_commits_for_midx_bitmap() within\nwrite_midx_bitmap(). If we free (and nullify) the array at that point,\nwe can free a decent chunk of memory before the bitmap logic adds more\nto the memory footprint.\n\nHere is the massif memory load chart after this change:\n\n    GB\n3.111^      #\n     |      #                              :::::::::::@::::::::::::::@\n     |      #        ::::::::::::::::::::::::: : :: : @:: ::::: :: ::@\n     |     @#  :::::::::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |     @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :::@#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n     |  :: @#::: ::: :::::: :::: :: : :::::::: : :: : @:: ::::: :: ::@\n   0 +--------------------------------------------------------------->\n\nThe previous change introduced a refactoring of write_midx_bitmap() to\nmake it more clear how much of the 'struct write_midx_context' instance\nis needed at different parts of the process. In addition, the following\ndefensive programming measures were put in place:\n\n 1. Using FREE_AND_NULL() we will at least get a segfault from reading a\n    NULL pointer instead of a use-after-free.\n\n 2. 'entries_nr' is also set to zero to make any loop that would iterate\n    over the entries be trivial.\n\n 3. Add significant comments in write_midx_internal() to add warnings\n    for future authors who might accidentally add references to this\n    cleared memory.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n midx.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex e2dd808b35d..772ab7d2944 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1451,6 +1451,15 @@ static int write_midx_internal(const char *object_dir,\n \n \t\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, &ctx);\n \n+\t\t/*\n+\t\t * The previous steps translated the information from\n+\t\t * 'entries' into information suitable for constructing\n+\t\t * bitmaps. We no longer need that array, so clear it to\n+\t\t * reduce memory pressure.\n+\t\t */\n+\t\tFREE_AND_NULL(ctx.entries);\n+\t\tctx.entries_nr = 0;\n+\n \t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &pdata,\n \t\t\t\t      commits, commits_nr, ctx.pack_order,\n \t\t\t\t      refs_snapshot, flags) < 0) {\n@@ -1459,6 +1468,10 @@ static int write_midx_internal(const char *object_dir,\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n+\t/*\n+\t * NOTE: Do not use ctx.entries beyond this point, since it might\n+\t * have been freed in the previous if block.\n+\t */\n \n \tif (ctx.m)\n \t\tclose_object_store(the_repository->objects);\n-- \ngitgitgadget\n"},{"id":"459378","messageId":"xmqq8ropccv3.fsf@gitster.g","threadId":"58183","inReplyTo":"98e72f71b6bec6f5c2df4139ca3df37d97ddcf54.1658244366.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] midx: reduce memory pressure while writing bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-19T15:59:28Z","receivedAt":"2022-07-19T15:59:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/midx.c b/midx.c\n> index e2dd808b35d..772ab7d2944 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1451,6 +1451,15 @@ static int write_midx_internal(const char *object_dir,\n>  \n>  \t\tcommits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, &ctx);\n>  \n> +\t\t/*\n> +\t\t * The previous steps translated the information from\n> +\t\t * 'entries' into information suitable for constructing\n> +\t\t * bitmaps. We no longer need that array, so clear it to\n> +\t\t * reduce memory pressure.\n> +\t\t */\n> +\t\tFREE_AND_NULL(ctx.entries);\n> +\t\tctx.entries_nr = 0;\n> +\n>  \t\tif (write_midx_bitmap(midx_name.buf, midx_hash, &pdata,\n>  \t\t\t\t      commits, commits_nr, ctx.pack_order,\n>  \t\t\t\t      refs_snapshot, flags) < 0) {\n\nAs the reduced helper, thanks to step [1/3], only takes the\npack_order[] array, without being even aware of other members in the\nctx struct, it is immediately obvious that this early freeing is\nsafe for this call.  It is a bit messy.\n\nI've been staring at the code and was wondering if we can just get\nrid of pack_order member from the context, and make pack_order a\nseparate local variable that belong to this function.  The separate\nvariable needs to be packaged together with ctx back to please the\nchunk-format API, so it may require more boilerplate code and may\nnot be an overall win.\n\n> @@ -1459,6 +1468,10 @@ static int write_midx_internal(const char *object_dir,\n>  \t\t\tgoto cleanup;\n>  \t\t}\n>  \t}\n> +\t/*\n> +\t * NOTE: Do not use ctx.entries beyond this point, since it might\n> +\t * have been freed in the previous if block.\n> +\t */\n\nOK.\n\n>  \tif (ctx.m)\n>  \t\tclose_object_store(the_repository->objects);\n\n"}]}