Skip to content

sbcl: Increase default heap size to 3GB.#298034

Closed
aadcg wants to merge 1 commit intoNixOS:masterfrom
aadcg:increase-default-heap-size-sbcl
Closed

sbcl: Increase default heap size to 3GB.#298034
aadcg wants to merge 1 commit intoNixOS:masterfrom
aadcg:increase-default-heap-size-sbcl

Conversation

@aadcg
Copy link

@aadcg aadcg commented Mar 22, 2024

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Mar 22, 2024

buildArgs = [
"--prefix=$out"
"--dynamic-space-size=3072"
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why? What is the reason behind changing the default?
  2. Make it configurable. Something like an argument dynamicSpaceSize ? 3 * 1024, plus something like
Suggested change
"--dynamic-space-size=3072"
"--dynamic-space-size=${dynamicSpaceSize}"

Copy link
Author

Choose a reason for hiding this comment

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

  1. The goal is to increase the default heap size when sbcl is launched, which defaults to 1GB. Given today's machines, it is an unfit default. Note that on Guix it also defaults to 3GB.

  2. The suggestion makes little sense since sbcl can be started via sbcl --dynamic-space-size <megabytes>. In other words, sbcl is already flexible enough in that regard.

Copy link
Member

Choose a reason for hiding this comment

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

It depends on how many SBCL's you want to launch at the same time (and is it nicer or not to skip the extra argument), so I'd say that if we want to override a default without a clear justification for an exact number, we should make it configurable.

Copy link
Member

Choose a reason for hiding this comment

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

  1. The goal is to increase the default heap size when sbcl is launched, which defaults to 1GB. Given today's machines, it is an unfit default. Note that on Guix it also defaults to 3GB.

I do not like the idea of changing defaults, but I find it acceptable here.

2. The suggestion makes little sense since `sbcl` can be started via `sbcl --dynamic-space-size <megabytes>`. In other words, `sbcl` is already flexible enough in that regard.

Not OK.
The argument "the user can set it by itself" can be used against your own PR.
In other words, your PR makes as little sense as my suggestion.

If your next argument will be "almost everyone sets it at 3GB, so let's do it ourselves", then the same can be said about some niche examples like "in my rig of machine learning we can set this to 64GB": the niche user can tweak an argument instead of overriding the code.

@ofborg ofborg bot requested review from 7c6f434c, Uthar, hraban, lukego and nagy March 22, 2024 13:41
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Mar 22, 2024
Copy link
Member

@hraban hraban left a comment

Choose a reason for hiding this comment

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

I am not a fan of changing the default [edit: in nixpkgs]. This should be taken up with SBCL devs, it's not for us to decide. They say 1GiB, we do 1GiB.

I do like the idea of allowing this to be more easily configured, but it should probably be a setuphook and a wrapper to do so, just as how nix controls e.g. GCC.

@aadcg
Copy link
Author

aadcg commented Mar 22, 2024

After taking all views into account, I believe that the best move is to close the PR. Thanks.

@aadcg aadcg closed this Mar 22, 2024
@aadcg aadcg deleted the increase-default-heap-size-sbcl branch March 22, 2024 20:38
@hraban
Copy link
Member

hraban commented Mar 22, 2024

After taking all views into account, I believe that the best move is to close the PR. Thanks.

Thanks for your PR though, and maybe it helps to know I constantly run into the 1GiB heap limit as well. Allowing for controlling the heap size through a setup hook has been on my todo list for ages so I completely empathize.

If you manage to convince upstream SBCL to change the default you have my +1.

@aadcg
Copy link
Author

aadcg commented Mar 28, 2024

See https://sourceforge.net/p/sbcl/mailman/message/58754345/.

@hraban hraban mentioned this pull request Apr 13, 2024
13 tasks
@hraban
Copy link
Member

hraban commented May 24, 2024

For the record: SBCL from Nix can now have its memory controlled using an envvar:

$ sbcl --script <<<'(format T "~A~%" (sb-kernel::dynamic-space-size))'
1073741824
$ NIX_SBCL_DYNAMIC_SPACE_SIZE=3gb sbcl --script <<<'(format T "~A~%" (sb-kernel::dynamic-space-size))'
3221225472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants