Update Gambit and Gerbil, fix #78921 #80224

fare · 2020-02-16T00:34:16Z

Motivation for this change

Upstream update + bug fix

Things done

7c6f434c · 2020-02-18T07:52:20Z

@GrahamcOfBorg build gambit-unstable gerbil-unstable

fare · 2020-02-19T08:59:11Z

Looks as good as it'll ever get... timeout on aa64 :-(

fare · 2020-02-21T09:10:23Z

While we're waiting, I just updated gerbil-unstable to the upstream

fare · 2020-02-21T09:13:27Z

@GrahamcOfBorg build gerbil-unstable

fare · 2020-02-25T00:23:23Z

@GrahamcOfBorg build gambit gerbil gambit-unstable gerbil-unstable

eraserhd · 2020-02-25T02:06:29Z

pkgs/development/compilers/gambit/build.nix

@@ -38,6 +38,10 @@ stdenv.mkDerivation rec {
      #--enable-inline-jumps
      #--enable-char-size=1" ; default is 4
    )
+    export CC=${gcc}/bin/gcc CXX=${gcc}/bin/g++ \


MacOS's stdenv uses clang by default. Depending on gcc means that using gambit might need to build all of gcc.

$CC should already be set, although without a full path, meaning you should be able to say export CC="$(command -v $CC)" CXX="$(command -v $CXX)" ... and so forth, and not depend on gcc. I did something like this for plan9port
here.

This way, the reference to gcc can be removed.

I don't remember how much slower clang-generated code it, but it's a significant factor. 200MB of space is cheap these days, speed is not. If you want a slow Scheme, there is plenty of competition to Gambit.

Can you try the two recipes and compare the generated code on some benchmarks?

And even if we pick clang, we probably want the path hardcoded with some CC= statement, instead of randomly inherited from the $PATH.

Note that the CC= statement, etc., is important, because IIUC, the values are persisted in the installation as default values.

(Thus gambit knows where to find a C toolchain that is compatible with what it used to compile its own code.)

I think I have seen comparisons of latest-at-the-time releases where Clang was generating noticeably faster code. For some input code bases and target CPUs… I think these comparisons coexisted with the opposite comparisons for the same compiler versions.

I would not be surprised if Clang is 10% faster (for the current versions in Nixpkgs). Or 10% slower.

Wait, GCC -O2 is also slower than GCC -O1? In addition to Clang taking longer than GCC to compile which is … not the expected situation. Maybe GCC-efficient support for unrestricted reenterable continuations leads to unique idioms.

Yes, now I need to compile with -O1, -Os, -O2 and see what works better. Until I do my own benchmark, I should probably follow Marc's advice and leave it at -O1.

Maybe it is even worth a comment that the Gambit codebase manages to violate the basic assumptions about the compiler performance and compiled-code performance…

I will, in my next PR (or this one, if still not merged), after I confirm the benchmarks...
It takes over 1h to run the benchmarks at https://github.com/ecraven/r7rs-benchmarks, plus maybe half an hour to recompile gambit, so say 2h per configuration, 4h for the next two configurations (-O1 and -Os)...

In my benchmarks, -Os clearly wins over both -O1 and -O2 most of the time (as far as the generated code goes), so that's what I'll use.

eraserhd

For reference, here's what I have so far, though I have quite a few more issues to work through:

https://github.com/NixOS/nixpkgs/compare/master...eraserhd:gambit-updates?expand=1

(Don't worry about conflicts, I'll rebase off your branch when I have time).

fare · 2020-02-25T02:58:05Z

@eraserhd I've integrated some of your suggestions in the current version of my branch.

For other ones... I need be convinced.

fare · 2020-02-25T04:09:12Z

Success! @7c6f434c or @thoughtpolice can you review and merge? (Thank you so much for your precious time!)

eraserhd · 2020-02-25T14:32:11Z

pkgs/development/compilers/gambit/build.nix

-      #--enable-char-size=1" ; default is 4
-    )
-    ./configure ''${options[@]}
+  postConfigure = ''


Locally, on NixOS, this does not run. You'll need to avoid overriding configurePhase, which you should be able to do by setting environment variables in preConfigure.

Not sure what you mean. The build recipe works for me and for the borg. What "does not run"?

I believe «does not run» is about just the postConfigure part. The build might succeed without this part being run.

Yes, what @7c6f434c said - the default code for configurePhase runs postConfigure. Setting configurePhase prevents it from running. https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L1002

OK, is it better now?

eraserhd · 2020-02-25T15:07:07Z

pkgs/development/compilers/gambit/bootstrap.nix

+    export CC=${gcc}/bin/gcc CXX=${gcc}/bin/g++ \
+           CPP=${gcc}/bin/cpp CXXCPP=${gcc}/bin/cpp LD=${gcc}/bin/ld \
+           XMKMF=${coreutils}/bin/false
+    unset CFLAGS LDFLAGS LIBS CPPFLAGS CXXFLAGS


What's this about?

Since we're using GCC anyway, we might take advantage of it to bootstrap 10x faster (as per the 2 year old benchmark I posted).

I mean unsetting CFLAGS, LDFLAGS, etc.

I'm pretty sure Nix doesn't set any of these in the first place, but what are you trying to avoid?

I suppose that is both a way to document which flags Gambit depends on, and a way to ensure it all works even in an impure nix shell?

eraserhd · 2020-02-26T21:47:41Z

pkgs/development/compilers/gambit/build.nix

+      --replace /usr/local/opt/openssl/lib "${openssl}/lib" \
+      --replace /usr/local/opt/openssl@1.1/lib "${openssl}/lib"


Suggested change

--replace /usr/local/opt/openssl/lib "${openssl}/lib" \

--replace /usr/local/opt/openssl@1.1/lib "${openssl}/lib"

--replace /usr/local/opt/openssl/lib "${openssl.out}/lib" \

--replace /usr/local/opt/openssl@1.1/lib "${openssl.out}/lib"

Without this suggestion:

jfelice@crunch /tmp $ gsc -verbose -exe foo.scm Parsing: "expr" "expr" Compiling: Dumping: #<primitive foo#> Compilation finished. /nix/store/1kn7fi3hhi33jms3113riyzwyn2yqpqd-gcc-wrapper-9.2.0/bin/gcc -Os -Wno-unused -Wno-write-strings -Wdisabled-optimization -fwrapv -fno-strict-aliasing -fno-trapping-math -fno-math-errno -fschedule-insns2 -fmodulo-sched -freschedule-modulo-scheduled-loops -fomit-frame-pointer -fPIC -fno-common -mpc64 -D___SINGLE_HOST -I"/nix/store/6w4rkrclxcdivy9jhc25zx6l166g2a13-gambit-4.9.3/include" -c -o "foo.o" foo.c /nix/store/1kn7fi3hhi33jms3113riyzwyn2yqpqd-gcc-wrapper-9.2.0/bin/gcc -Os -Wno-unused -Wno-write-strings -Wdisabled-optimization -fwrapv -fno-strict-aliasing -fno-trapping-math -fno-math-errno -fschedule-insns2 -fmodulo-sched -freschedule-modulo-scheduled-loops -fomit-frame-pointer -fPIC -fno-common -mpc64 -D___SINGLE_HOST -I"/nix/store/6w4rkrclxcdivy9jhc25zx6l166g2a13-gambit-4.9.3/include" -c -o "foo_.o" foo_.c /nix/store/1kn7fi3hhi33jms3113riyzwyn2yqpqd-gcc-wrapper-9.2.0/bin/gcc -Wno-unused -Wno-write-strings -Wdisabled-optimization -fwrapv -fno-strict-aliasing -fno-trapping-math -fno-math-errno -fschedule-insns2 -fmodulo-sched -freschedule-modulo-scheduled-loops -fomit-frame-pointer -fPIC -fno-common -mpc64 -rdynamic -D___SINGLE_HOST -I"/nix/store/6w4rkrclxcdivy9jhc25zx6l166g2a13-gambit-4.9.3/include" -o "foo" foo_.o foo.o "/nix/store/6w4rkrclxcdivy9jhc25zx6l166g2a13-gambit-4.9.3/lib/libgambit.so" -lutil -ldl -lm -L/nix/store/7i3k8k854al6ci160syrq2xld97kz4ci-openssl-1.1.1d-bin/lib -lssl -lcrypto /nix/store/ffi3j388c39qbbskfqg1zw0x2lmbzhnk-binutils-2.31.1/bin/ld: cannot find -lssl /nix/store/ffi3j388c39qbbskfqg1zw0x2lmbzhnk-binutils-2.31.1/bin/ld: cannot find -lcrypto collect2: error: ld returned 1 exit status *** ERROR IN ##main -- target link failed while linking "/tmp/foo_.o" "/tmp/foo.o"

With this suggestion:

jfelice@crunch /tmp $ gsc -verbose -exe foo.scm Parsing: "expr" "expr" Compiling: Dumping: #<primitive foo#> Compilation finished. /nix/store/1kn7fi3hhi33jms3113riyzwyn2yqpqd-gcc-wrapper-9.2.0/bin/gcc -Os -Wno-unused -Wno-write-strings -Wdisabled-optimization -fwrapv -fno-strict-aliasing -fno-trapping-math -fno-math-errno -fschedule-insns2 -fmodulo-sched -freschedule-modulo-scheduled-loops -fomit-frame-pointer -fPIC -fno-common -mpc64 -D___SINGLE_HOST -I"/nix/store/9h6nzjy120q0ls0srfb6fr4iw26b6afm-gambit-4.9.3/include" -c -o "foo.o" foo.c /nix/store/1kn7fi3hhi33jms3113riyzwyn2yqpqd-gcc-wrapper-9.2.0/bin/gcc -Os -Wno-unused -Wno-write-strings -Wdisabled-optimization -fwrapv -fno-strict-aliasing -fno-trapping-math -fno-math-errno -fschedule-insns2 -fmodulo-sched -freschedule-modulo-scheduled-loops -fomit-frame-pointer -fPIC -fno-common -mpc64 -D___SINGLE_HOST -I"/nix/store/9h6nzjy120q0ls0srfb6fr4iw26b6afm-gambit-4.9.3/include" -c -o "foo_.o" foo_.c /nix/store/1kn7fi3hhi33jms3113riyzwyn2yqpqd-gcc-wrapper-9.2.0/bin/gcc -Wno-unused -Wno-write-strings -Wdisabled-optimization -fwrapv -fno-strict-aliasing -fno-trapping-math -fno-math-errno -fschedule-insns2 -fmodulo-sched -freschedule-modulo-scheduled-loops -fomit-frame-pointer -fPIC -fno-common -mpc64 -rdynamic -D___SINGLE_HOST -I"/nix/store/9h6nzjy120q0ls0srfb6fr4iw26b6afm-gambit-4.9.3/include" -o "foo" foo_.o foo.o "/nix/store/9h6nzjy120q0ls0srfb6fr4iw26b6afm-gambit-4.9.3/lib/libgambit.so" -lutil -ldl -lm -L/nix/store/i8l8gcnz3jya1dr4205gbb92akay3f1f-openssl-1.1.1d/lib -lssl -lcrypto jfelice@crunch /tmp $ ./foo hello, world

(it tries to find the libs in the bin package)

Yup. I saw that and I fixed it. The naming of package outputs can be confusing.

Use -Os rather than -O2 as our compilation flag, document why. Document why we always use gcc over clang. Fix openssl path in gambit. Stop trying to make static openssl.

fare · 2020-02-27T15:50:21Z

It's approved and passes tests... who's role is it to merge?

7c6f434c · 2020-02-27T16:51:19Z

Givn that @eraserhd has made a lot of helpful comments on the previous versions of this, I personally will only merge about some times has elapsed in case there are some further remarks. Hopefully I will come back to it when looking at the recently-participated list in a day or two.

Let Gerbil Scheme find its GERBIL_HOME where Nix put it when the environment variable is left unspecified. Comment out work in progress for static linking. Notes about working on macOS.

ofborg bot requested review from 7c6f434c and thoughtpolice February 16, 2020 01:42

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 16, 2020

fare force-pushed the fare branch from 94f7461 to 59e1b3a Compare February 21, 2020 09:09

fare mentioned this pull request Feb 25, 2020

gambit should use the gcc it was compiled with #78921

Closed

fare force-pushed the fare branch from 59e1b3a to 3e558f8 Compare February 25, 2020 00:22

fare force-pushed the fare branch 2 times, most recently from 7c90803 to 1260afa Compare February 25, 2020 01:33

eraserhd reviewed Feb 25, 2020

View reviewed changes

fare force-pushed the fare branch 2 times, most recently from 46b403e to c240182 Compare February 25, 2020 02:57

NixOS deleted a comment from fare Feb 25, 2020

7c6f434c approved these changes Feb 25, 2020

View reviewed changes

eraserhd reviewed Feb 25, 2020

View reviewed changes

fare force-pushed the fare branch from c240182 to fffc2d1 Compare February 26, 2020 03:24

ofborg bot requested a review from 7c6f434c February 26, 2020 03:34

eraserhd reviewed Feb 26, 2020

View reviewed changes

fare added 2 commits February 26, 2020 20:37

gambit: set CC to full path, fixes NixOS#78921

0d422d5

Use -Os rather than -O2 as our compilation flag, document why. Document why we always use gcc over clang. Fix openssl path in gambit. Stop trying to make static openssl.

gambit-unstable: 2019-07-21 -> 2020-02-24

32b3758

fare force-pushed the fare branch from fffc2d1 to ca0dfad Compare February 27, 2020 01:37

fare force-pushed the fare branch from ca0dfad to f8d3adf Compare February 27, 2020 02:05

7c6f434c approved these changes Feb 27, 2020

View reviewed changes

gerbil-unstable: 2019-11-15 -> 2020-02-27

4ee219e

Let Gerbil Scheme find its GERBIL_HOME where Nix put it when the environment variable is left unspecified. Comment out work in progress for static linking. Notes about working on macOS.

fare force-pushed the fare branch from f8d3adf to 4ee219e Compare February 27, 2020 21:03

ofborg bot requested a review from 7c6f434c February 27, 2020 21:13

7c6f434c merged commit 3ee1e3b into NixOS:master Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Gambit and Gerbil, fix #78921 #80224

Update Gambit and Gerbil, fix #78921 #80224

fare commented Feb 16, 2020

7c6f434c commented Feb 18, 2020

fare commented Feb 19, 2020

fare commented Feb 21, 2020

fare commented Feb 21, 2020

fare commented Feb 25, 2020

eraserhd Feb 25, 2020

fare Feb 25, 2020

fare Feb 25, 2020

fare Feb 25, 2020

7c6f434c Feb 25, 2020

7c6f434c Feb 25, 2020

fare Feb 25, 2020

7c6f434c Feb 25, 2020

fare Feb 25, 2020

fare Feb 26, 2020

eraserhd left a comment

fare commented Feb 25, 2020

fare commented Feb 25, 2020

eraserhd Feb 25, 2020

fare Feb 25, 2020

7c6f434c Feb 25, 2020

eraserhd Feb 25, 2020

fare Feb 26, 2020

eraserhd Feb 25, 2020

fare Feb 25, 2020

eraserhd Feb 25, 2020

fare Feb 26, 2020

eraserhd Feb 26, 2020

fare Feb 27, 2020

fare Feb 27, 2020

fare commented Feb 27, 2020

7c6f434c commented Feb 27, 2020

		--replace /usr/local/opt/openssl/lib "${openssl}/lib" \
		--replace /usr/local/opt/openssl@1.1/lib "${openssl}/lib"

Update Gambit and Gerbil, fix #78921 #80224

Update Gambit and Gerbil, fix #78921 #80224

Conversation

fare commented Feb 16, 2020

Motivation for this change

Things done

7c6f434c commented Feb 18, 2020

fare commented Feb 19, 2020

fare commented Feb 21, 2020

fare commented Feb 21, 2020

fare commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eraserhd left a comment

Choose a reason for hiding this comment

fare commented Feb 25, 2020

fare commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fare commented Feb 27, 2020

7c6f434c commented Feb 27, 2020