sbcl: read memory from envvar

[hraban]

Description of changes

Allow controlling SBCL memory through a SBCL_DYNAMIC_SPACE_SIZE envvar which has the same format as the --dynamic-space-size cli flag.

I have tried this locally and on https://github.com/hraban/cl-nix-lite and it significantly simplifies a few hokey workarounds I had. It might also address the concerns from #298034 depending on their circumstances. And would it help clean up the ugly wrapping we do in all-packages.nix at the moment? https://github.com/NixOS/nixpkgs/blob/200e237951a35168aff3af66965c08667b4833af/pkgs/top-level/all-packages.nix#L25547 etc.

The idea being that memory is set from the first value found in this order:

1. from the command line args
2. from the env
3. from embedded args in a core file
4. from defaults

Notably this means that memory can be set on a per-package basis and big packages can reuse build cache from smaller packages and vice-versa. Concretely: in cl-nix-lite the 3d-math package requires >3GiB of heap to build, but this meant setting the dynamic space size to 3GiB for all packages built by SBCL on CI, effectively disabling the build cache for SBCL.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[7c6f434c]

Looks good, actually. I guess I should test it on Linux, though.

In general this is basically what we try to achieve with any of the flexible wrappers in any case…

[hraban]

@7c6f434c I didn't get any traction on this patch on sbcl-devel ("https://sourceforge.net/p/sbcl/mailman/sbcl-devel/thread/2cf20df7-01d0-44f2-8551-0df01fe55f1a%400brg.net/"). Shall we prefix the envvar with NIX_ and keep this patch alive just in nixpkgs?

[hraban]

also pinging @catap I think you've been active around this stuff in guix and you might be interested?

[7c6f434c]

Yeah, we probably don't consider «withPackages» mandatory enough to put this there, and the patch looks fine to me.

[hraban]

@aadcg how does this sound to you ? does this help you at all?

[aadcg]

@hraban seems reasonable to me, thanks!

[hraban]

@7c6f434c open questions for you:

• should we prefix it with NIX_ ? In the spirit of https://datatracker.ietf.org/doc/html/rfc6648 I'm tempted not to do so because the envvar name is specific enough on its own.
• Should we override the embedded memory params on an executable core? When dumping an executable core you can explicitly include the memory parameters, and when the core starts it will reuse those instead of the default. I'm tempted to let the envvar override this, because you can already override them using explicit command line arguments, and the code has this comment:

      /* Our arg parsing isn't (and can't be) integrated with the application's,
       * but we really want users to be able to override the heap size.
       * So don't parse most options, but _do_ parse memory size options and/or
       * core page merging options, wherever they occur, and strip them out.
       * Any args that remain are passed through to Lisp.
       *
       * This does have a small semantic glitch: If your executable accepts
       * flags such as "--my-opt" "--merge-core-pages" where "--merge-core-pages"
       * is literally (and perversely) the value the user gives to "--my-opt",
       * that's just too bad! The somewhat conventional "--" option will stop
       * parsing SBCL options and pass everything else through including the "--".
       * The rationale for passing "--" through is that we're trying to be
       * as uninvasive as possible. Let's hope that nobody needs to put a "--"
       * to the left of any of the memory size options */

My intuition is to leave both of these as-is but I'm curious what you think.

I also added some unit tests to this PR for the patched behavior.

[7c6f434c]

Re: overriding whatever command-line allows to override: very much yes

Re: X-: I do not think this applies here, as NIX_ is used in Nixpkgs to specify the project introducing the variable (X- is not doing any real namespacing, but the RFC explicitly says that creator-namespace is another story). I do sometimes grep env output for NIX_…

Or do you have a strong preference for no NIX_ anyway?

[hraban]

Fair, to be honest I think doing bare SBCL_... has only cosmetic benefits, while doing NIX_SBCL_... has the benefit of making it clear to anyone maintaining relevant infra that this will only work for SBCL built with Nix. If I imagine finding e.g. CLOJURE_MEM_SIZE in some config , I'd be annoyed to later find out it's Nix only.

[hraban]

Running full cl-nix-lite test-suite here https://github.com/hraban/cl-nix-lite/actions/runs/9037546275 if that passes we should be good.

[catap]

also pinging @catap I think you've been active around this stuff in guix and you might be interested?

thanks for ping!

I never used guix but I've reimplemented something similar for lisp inside MacPorts.

[hraban]

also pinging @catap I think you've been active around this stuff in guix and you might be interested?

thanks for ping!

I never used guix but I've reimplemented something similar for lisp inside MacPorts.

Nice, I'd love to see the patch. Do you have a link?

[catap]

On Fri, 10 May 2024 21:26:49 +0100, Hraban ***@***.***> wrote: > I never used guix but I've reimplemented something similar for lisp inside MacPorts. Nice, I'd love to see the patch. Do you have a link?

Similar means "another lisp package manager" like guix or nix if we compare it with quicklisp. Not a similar patch. Sorry for confusion.

…

-- wbr, Kirill

[7c6f434c]

@hraban I do not understand the title of the linked job; overall — is there any pending testing? I am fine merging now.

[hraban]

It all seems to work great 👍 I say it's ready to merge

[vcunat]

There apparently was some interaction between staging* changes (PR #309297) and this PR, resulting into sbcl build being broken on x86_64-linux (and thus all of sbclPackages).

The failure "first appeared" on an automatic merge from master to staging-next (42828a7). The packages build and are in cache.nixos.org for both parents, but broken at that commit - and that's how it's in the current nixpkgs master now :-/

(I was now testing with sbclPackages.cl-colors2 in particular, but sbcl build itself gets broken.)
The automatic merge happened after the last staging-next eval on Hydra, so I didn't notice.

[vcunat]

I'm not sure what to do here. If you have no ideas, I'd probably suggest to revert this merge until more is known.

[vcunat]

Oh, after a few retries the build did succeed 🤦🏽 so I don't know.

[7c6f434c]

Do you have the failed log before retries in easy access? I guess I could at least try to see if there is an obvious reason for flakiness…

[vcunat]

No, I'm afraid I don't know of a log now. I wasn't careful.

[7c6f434c]

No problem and moving forward the filling of the cache is always valuable! It was just an «if it's cheap, why not try» question. I guess sooner or later we will hit the same builder again … (If it is something e.g. about CPU generations and availability of some instructions)

[vcunat]

t4b machine has had both success and failure on the very same derivation.

[7c6f434c]

Ah interesting, thanks!

[vcunat]

It's unfortunate. Otherwise I'd have the log.