Skip to content
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

improve --heap-size-hint arg handling #48050

Merged
merged 15 commits into from
Feb 23, 2024
Merged

improve --heap-size-hint arg handling #48050

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c4c5229
improve heap-size-hint arg handling
mbauman Dec 30, 2022
da564a7
more testsets
mbauman Dec 30, 2022
e7fb5dd
less undefined behavior, more debugging
mbauman Jan 4, 2023
84cbd0e
better debugging
mbauman Jan 5, 2023
fa2d281
long double consistently
mbauman Jan 5, 2023
3b889d4
Merge branch 'master' into mb/heap_size_hint_cleanup
gbaraldi Jan 16, 2023
35dc093
Merge remote-tracking branch 'origin/master' into HEAD
mbauman Feb 8, 2024
5f32478
remove debugging code
mbauman Feb 8, 2024
7495e55
match verbiage with #50480
mbauman Feb 8, 2024
2974896
standardize documentation
mbauman Feb 9, 2024
1291220
use utf8proc_tolower
mbauman Feb 12, 2024
a7e1bb3
Merge branch 'master' into mb/heap_size_hint_cleanup
vtjnash Feb 13, 2024
07fbf57
use a static ascii_tolower
mbauman Feb 19, 2024
3de28b5
Merge remote-tracking branch 'origin/master' into mb/heap_size_hint_c…
mbauman Feb 20, 2024
b902aab
Merge branch 'master' into mb/heap_size_hint_cleanup
mbauman Feb 21, 2024
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
60 changes: 33 additions & 27 deletions src/jloptions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "julia_internal.h"

#include <unistd.h>
#include <ctype.h>
#include <getopt.h>
#include "julia_assert.h"

Expand Down Expand Up @@ -182,8 +183,8 @@ static const char opts[] =
" fallbacks to the latest compatible BugReporting.jl if not. For more information, see\n"
" --bug-report=help.\n\n"

" --heap-size-hint=<size> Forces garbage collection if memory usage is higher than that value.\n"
" The memory hint might be specified in megabytes(500M) or gigabytes(1G)\n\n"
" --heap-size-hint=<size> Forces garbage collection if memory usage is greater than the given number of bytes.\n"
" The size may be specified with units like 500M (megabytes) or 2.5GB (gigabytes)\n\n"
mbauman marked this conversation as resolved.
Show resolved Hide resolved
;

static const char opts_hidden[] =
Expand Down Expand Up @@ -781,33 +782,38 @@ JL_DLLEXPORT void jl_parse_opts(int *argcp, char ***argvp)
break;
case opt_heap_size_hint:
if (optarg != NULL) {
size_t endof = strlen(optarg);
long double value = 0.0;
if (sscanf(optarg, "%Lf", &value) == 1 && value > 1e-7) {
char unit = optarg[endof - 1];
uint64_t multiplier = 1ull;
switch (unit) {
case 'k':
case 'K':
multiplier <<= 10;
break;
case 'm':
case 'M':
multiplier <<= 20;
break;
case 'g':
case 'G':
multiplier <<= 30;
break;
case 't':
case 'T':
multiplier <<= 40;
break;
default:
break;
}
jl_options.heap_size_hint = (uint64_t)(value * multiplier);
char unit[4] = {0};
int nparsed = sscanf(optarg, "%Lf%3s", &value, unit);
if (nparsed == 0 || strlen(unit) > 2 || (strlen(unit) == 2 && tolower(unit[1]) != 'b')) {
jl_errorf("julia: invalid argument to --heap-size-hint (%s)", optarg);
}
uint64_t multiplier = 1ull;
switch (tolower(unit[0])) {
mbauman marked this conversation as resolved.
Show resolved Hide resolved
case '\0':
case 'b':
break;
case 'k':
multiplier <<= 10;
break;
case 'm':
multiplier <<= 20;
break;
case 'g':
multiplier <<= 30;
break;
case 't':
multiplier <<= 40;
break;
default:
jl_errorf("julia: invalid argument to --heap-size-hint (%s)", optarg);
break;
}
long double sz = value * multiplier;
if (isnan(sz) || sz < 0) {
jl_errorf("julia: invalid argument to --heap-size-hint (%s)", optarg);
}
jl_options.heap_size_hint = sz < UINT64_MAX ? (uint64_t)sz : UINT64_MAX;
}
if (jl_options.heap_size_hint == 0)
jl_errorf("julia: invalid argument to --heap-size-hint without memory size specified");
Expand Down
23 changes: 23 additions & 0 deletions test/cmdlineargs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,3 +828,26 @@ end
@test lines[4] == "bar"
end
end

@testset "--heap-size-hint" begin
exename = `$(Base.julia_cmd())`
@test errors_not_signals(`$exename --heap-size-hint -e "exit(0)"`)
@testset "--heap-size-hint=$str" for str in ["asdf","","0","1.2vb","b","GB","1.2gb2","42gigabytes","5gig","2GiB","NaNt"]
@test errors_not_signals(`$exename --heap-size-hint=$str -e "exit(0)"`)
if !errors_not_signals(`$exename --heap-size-hint=$str -e "exit(0)"`)
@show str
try
run(`$exename --heap-size-hint=$str -e "exit(0)"`)
catch
end
mbauman marked this conversation as resolved.
Show resolved Hide resolved
end
mbauman marked this conversation as resolved.
Show resolved Hide resolved
end
k = 1024
m = 1024k
g = 1024m
t = 1024g
@testset "--heap-size-hint=$str" for (str, val) in [("1", 1), ("1e7", 1e7), ("2.5e7", 2.5e7), ("1MB", 1m), ("2.5g", 2.5g), ("1e4kB", 1e4k),
("1e100", typemax(UInt64)), ("1e500g", typemax(UInt64)), ("1e-12t", 1), ("500000000b", 500000000)]
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, clearly I did not look closely enough before: I thought this was about adding support for inputs like 2.5g which seems reasonable to me

But is a heap size hint of the form 1e-12t or 1e100 really something we want to / should support?

I guess it would be more work to not support them, though. Ah well.

Copy link
Member Author

@mbauman mbauman Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This PR doesn't change the numeric syntaxes supported here — it just adds tests for them.

The big change here isn't about the supported numeric values, but rather the byte unit. Before Julia would silently discard/ignore unrecognized units. After this PR, Julia errors. That's the big improvement.

Currently:

▶ julia --heap-size-hint=2.5GB -E "Base.JLOptions().heap_size_hint"
0x0000000000000002

▶ julia --heap-size-hint=2.5Gibberish -E "Base.JLOptions().heap_size_hint"
0x0000000000000002

▶ julia --heap-size-hint=2.5Gobbledeegook -E "Base.JLOptions().heap_size_hint"
0x0000000000000a00

After:

▶ ./julia --heap-size-hint=2.5GB -E "Base.JLOptions().heap_size_hint"
0x00000000a0000000

▶ ./julia --heap-size-hint=2.5Gibberish -E "Base.JLOptions().heap_size_hint"
ERROR: julia: invalid argument to --heap-size-hint (2.5Gibberish)

▶ ./julia --heap-size-hint=2.5Gobbledeegook -E "Base.JLOptions().heap_size_hint"
ERROR: julia: invalid argument to --heap-size-hint (2.5Gobbledeegook)

@test parse(UInt64,read(`$exename --heap-size-hint=$str -E "Base.JLOptions().heap_size_hint"`, String)) == val
end
end