Skip to content

range-diff: add configurable memory limit for cost matrix #1958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

range-diff: add configurable memory limit for cost matrix #1958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions builtin/log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,6 +1404,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
struct range_diff_options range_diff_opts = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Thu, Aug 28, 2025 at 2:00 AM pcasaretto via GitGitGadget
<[email protected]> wrote:
>
> From: pcasaretto <[email protected]>
> Signed-off-by: Paulo Casaretto <[email protected]>

The names (and emails) in these should match; I believe the name in
the From field is set by Gitgitgadget based on your profile settings;
see https://github.com/settings/profile and set your name there.

>  static void get_correspondences(struct string_list *a, struct string_list *b,
> -                               int creation_factor)
> +                               int creation_factor, size_t max_memory)
>  {
>         int n = a->nr + b->nr;
>         int *cost, c, *a2b, *b2a;
>         int i, j;
> -
> -       ALLOC_ARRAY(cost, st_mult(n, n));
> +       size_t cost_size = st_mult(n, n);
> +       size_t cost_bytes = st_mult(sizeof(int), cost_size);
> +       if (cost_bytes >= max_memory) {
> +               struct strbuf cost_str = STRBUF_INIT;
> +               struct strbuf max_str = STRBUF_INIT;
> +               strbuf_humanise_bytes(&cost_str, cost_bytes);
> +               strbuf_humanise_bytes(&max_str, max_memory);
> +               die(_("range-diff: unable to compute the range-diff, since it "
> +                     "exceeds the maximum memory for the cost matrix: %s "
> +                     "(%"PRIuMAX" bytes) needed, %s (%"PRIuMAX" bytes) available"),

available?  I'm worried the error message will report in users
checking system memory, claiming they have 14GB available on their
system, and then reporting a "bug".

Perhaps something like:

+                     "(%"PRIuMAX" bytes) needed, limited to %s
(%"PRIuMAX" bytes)"),

?


The rest of the patch looks good to me.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <[email protected]> writes:

> <[email protected]> wrote:
>>
>> From: pcasaretto <[email protected]>
>> Signed-off-by: Paulo Casaretto <[email protected]>
>
> The names (and emails) in these should match; I believe the name in
> the From field is set by Gitgitgadget based on your profile settings;
> see https://github.com/settings/profile and set your name there.
>
>>  static void get_correspondences(struct string_list *a, struct string_list *b,
>> -                               int creation_factor)
>> +                               int creation_factor, size_t max_memory)
>>  {
>>         int n = a->nr + b->nr;
>>         int *cost, c, *a2b, *b2a;
>>         int i, j;
>> -
>> -       ALLOC_ARRAY(cost, st_mult(n, n));
>> +       size_t cost_size = st_mult(n, n);
>> +       size_t cost_bytes = st_mult(sizeof(int), cost_size);
>> +       if (cost_bytes >= max_memory) {
>> +               struct strbuf cost_str = STRBUF_INIT;
>> +               struct strbuf max_str = STRBUF_INIT;
>> +               strbuf_humanise_bytes(&cost_str, cost_bytes);
>> +               strbuf_humanise_bytes(&max_str, max_memory);
>> +               die(_("range-diff: unable to compute the range-diff, since it "
>> +                     "exceeds the maximum memory for the cost matrix: %s "
>> +                     "(%"PRIuMAX" bytes) needed, %s (%"PRIuMAX" bytes) available"),
>
> available?  I'm worried the error message will report in users
> checking system memory, claiming they have 14GB available on their
> system, and then reporting a "bug".
>
> Perhaps something like:
>
> +                     "(%"PRIuMAX" bytes) needed, limited to %s
> (%"PRIuMAX" bytes)"),

Sounds like a good idea.

I am not a huge fan of configuration variables that do not have a
command line option.  Assuming that it is not like you'd be doing
overly huge range-diff that would not fit your memory every day,
shouldn't we start this with a command line option without a
configuration variable to gauge how useful it would be for users
with such a need, and then after it proves useful and we identify a
workflow where a user would be passing this option all the time, add
a configuration to allow it always be in effect (with command line
override still available)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Thu, Aug 28, 2025 at 2:22 PM Junio C Hamano <[email protected]> wrote:
>
> I am not a huge fan of configuration variables that do not have a
> command line option.  Assuming that it is not like you'd be doing
> overly huge range-diff that would not fit your memory every day,
> shouldn't we start this with a command line option without a
> configuration variable to gauge how useful it would be for users
> with such a need, and then after it proves useful and we identify a
> workflow where a user would be passing this option all the time, add
> a configuration to allow it always be in effect (with command line
> override still available)?

Isn't that what Paulo's patch does?  Maybe I'm just blind, but I've
looked over the patch a couple times and don't see where he's reading
from a configuration variable; am I just missing it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <[email protected]> writes:

> On Thu, Aug 28, 2025 at 2:22 PM Junio C Hamano <[email protected]> wrote:
>>
>> I am not a huge fan of configuration variables that do not have a
>> command line option.  Assuming that it is not like you'd be doing
>> overly huge range-diff that would not fit your memory every day,
>> shouldn't we start this with a command line option without a
>> configuration variable to gauge how useful it would be for users
>> with such a need, and then after it proves useful and we identify a
>> workflow where a user would be passing this option all the time, add
>> a configuration to allow it always be in effect (with command line
>> override still available)?
>
> Isn't that what Paulo's patch does?  Maybe I'm just blind, but I've
> looked over the patch a couple times and don't see where he's reading
> from a configuration variable; am I just missing it?

Ah, I just blindly trusted that the "configurable memory limit" on
the subject line is talking about configuring memory limit with some
mechanism.  Thanks for correcting me.

.creation_factor = rev->creation_factor,
.dual_color = 1,
.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,
.diffopt = &opts,
.other_arg = &other_arg
};
Expand Down
21 changes: 21 additions & 0 deletions builtin/range-diff.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "parse-options.h"
#include "range-diff.h"
#include "config.h"
#include "parse.h"


static const char * const builtin_range_diff_usage[] = {
Expand All @@ -15,6 +16,21 @@ N_("git range-diff [<options>] <base> <old-tip> <new-tip>"),
NULL
};

static int parse_max_memory(const struct option *opt, const char *arg, int unset)
{
size_t *max_memory = opt->value;
uintmax_t val;

if (unset)
return 0;

if (!git_parse_unsigned(arg, &val, SIZE_MAX))
return error(_("invalid max-memory value: %s"), arg);

*max_memory = (size_t)val;
return 0;
}

int cmd_range_diff(int argc,
const char **argv,
const char *prefix,
Expand All @@ -25,6 +41,7 @@ int cmd_range_diff(int argc,
struct strvec diff_merges_arg = STRVEC_INIT;
struct range_diff_options range_diff_opts = {
.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT,
.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,
.diffopt = &diffopt,
.other_arg = &other_arg
};
Expand All @@ -40,6 +57,10 @@ int cmd_range_diff(int argc,
PARSE_OPT_OPTARG),
OPT_PASSTHRU_ARGV(0, "diff-merges", &diff_merges_arg,
N_("style"), N_("passed to 'git log'"), 0),
OPT_CALLBACK(0, "max-memory", &range_diff_opts.max_memory,
N_("size"),
N_("maximum memory for cost matrix (default 4G)"),
parse_max_memory),
OPT_PASSTHRU_ARGV(0, "remerge-diff", &diff_merges_arg, NULL,
N_("passed to 'git log'"), PARSE_OPT_NOARG),
OPT_BOOL(0, "left-only", &left_only,
Expand Down
1 change: 1 addition & 0 deletions log-tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,7 @@ static void show_diff_of_diff(struct rev_info *opt)
struct range_diff_options range_diff_opts = {
.creation_factor = opt->creation_factor,
.dual_color = 1,
.max_memory = RANGE_DIFF_MAX_MEMORY_DEFAULT,
.diffopt = &opts
};

Expand Down
20 changes: 16 additions & 4 deletions range-diff.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,13 +325,24 @@ static int diffsize(const char *a, const char *b)
}

static void get_correspondences(struct string_list *a, struct string_list *b,
int creation_factor)
int creation_factor, size_t max_memory)
{
int n = a->nr + b->nr;
int *cost, c, *a2b, *b2a;
int i, j;

ALLOC_ARRAY(cost, st_mult(n, n));
size_t cost_size = st_mult(n, n);
size_t cost_bytes = st_mult(sizeof(int), cost_size);
if (cost_bytes >= max_memory) {
struct strbuf cost_str = STRBUF_INIT;
struct strbuf max_str = STRBUF_INIT;
strbuf_humanise_bytes(&cost_str, cost_bytes);
strbuf_humanise_bytes(&max_str, max_memory);
die(_("range-diff: unable to compute the range-diff, since it "
"exceeds the maximum memory for the cost matrix: %s "
"(%"PRIuMAX" bytes) needed, limited to %s (%"PRIuMAX" bytes)"),
cost_str.buf, (uintmax_t)cost_bytes, max_str.buf, (uintmax_t)max_memory);
}
ALLOC_ARRAY(cost, cost_size);
ALLOC_ARRAY(a2b, n);
ALLOC_ARRAY(b2a, n);

Expand Down Expand Up @@ -591,7 +602,8 @@ int show_range_diff(const char *range1, const char *range2,
if (!res) {
find_exact_matches(&branch1, &branch2);
get_correspondences(&branch1, &branch2,
range_diff_opts->creation_factor);
range_diff_opts->creation_factor,
range_diff_opts->max_memory);
output(&branch1, &branch2, range_diff_opts);
}

Expand Down
5 changes: 5 additions & 0 deletions range-diff.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
#include "strvec.h"

#define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
#define RANGE_DIFF_MAX_MEMORY_DEFAULT \
(sizeof(void*) >= 8 ? \
((size_t)(1024L * 1024L) * (size_t)(4L * 1024L)) : /* 4GB on 64-bit */ \
((size_t)(1024L * 1024L) * (size_t)(2L * 1024L))) /* 2GB on 32-bit */

/*
* A much higher value than the default, when we KNOW we are comparing
Expand All @@ -17,6 +21,7 @@ struct range_diff_options {
unsigned dual_color:1;
unsigned left_only:1, right_only:1;
unsigned include_merges:1;
size_t max_memory;
const struct diff_options *diffopt; /* may be NULL */
const struct strvec *other_arg; /* may be NULL */
};
Expand Down
Loading