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.

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

Added JANET_NO_AMALG flag to Makefile #1175

Merged
merged 1 commit into from
Jun 4, 2023
Merged

Conversation

zevv
Copy link
Contributor

@zevv zevv commented Jun 2, 2023

(Not sure if people are willing to pull in this added complexity, but giving it a shot anyway)

This change allows building Janet from the individual source files instead of from the amalgamated janet.c; this considerably speeds up parallel builds on modern CPUs;

 make clean; time make -j NO_AMALG=0 
[...]
real	0m7.882s

vs

make clean; time make -j NO_AMALG=1 
[...]
real	0m1.481s

For me this make the difference between "comfortably and quickly running my tests" and "getting slightly annoyed staring at my screen",

@sogaiu
Copy link
Contributor

sogaiu commented Jun 3, 2023

The speedup is nice 👍

@bakpakin
Copy link
Member

bakpakin commented Jun 3, 2023

The added complexity is ok, since it is not much added complexity. We used to have this, however the requirement for distributing as janet.c and janet.h meant that sometimes the amalgamation would break, so I made the amalgamation the default in the Makefile which reduced that problem.

But a lot has changed and stabilized, I guess I just got use to waiting for the compiler 😅

@disruptek
Copy link

It would probably be sufficient to make the amalgamation a build/ci artifact, right?

@bakpakin
Copy link
Member

bakpakin commented Jun 4, 2023

Yes, but the idea was to have one fewer way to build things and I would get PRs that would break the amalgamated build (and break them myself). Basically, it was just one less thing to think about. Still, it's like a breath of fresh air to have incremental compilation nice and fast again.

@zevv
Copy link
Contributor Author

zevv commented Jun 4, 2023

Is the current implementation acceptable, in particular the way the option is passed to and handled in boot. janet? Also maybe change NO_AMALG into something nicer like JANET_DISABLE_AMALGAMATION? (I will not be typing that more then once (in my .bashrc only)

@sogaiu
Copy link
Contributor

sogaiu commented Jun 4, 2023

How about JANET_NO_AMALG?

@bakpakin
Copy link
Member

bakpakin commented Jun 4, 2023

Current implementation looks good, but it is probably better to have a name that is unlikely to collide with anything as the trigger to avoid surprises.

source files instead of from the amalgamated janet.c; this considerably
speeds up parallel builds on modern CPUs
@zevv zevv changed the title Added NO_AMALG flag to Makefile Added JANET_NO_AMALG flag to Makefile Jun 4, 2023
@zevv
Copy link
Contributor Author

zevv commented Jun 4, 2023

env var changed to JANET_NO_AMALG

@zevv zevv marked this pull request as ready for review June 4, 2023 18:03
@CosmicToast
Copy link
Contributor

I'm curious on what the speed delta would look like when running with a more parallelized linker. (e.g mold)

@zevv
Copy link
Contributor Author

zevv commented Jun 4, 2023

Linking really is not the problem, that's typically done in tens of milliseconds for a project this size.

@bakpakin bakpakin merged commit 649173f into janet-lang:master Jun 4, 2023
@sogaiu
Copy link
Contributor

sogaiu commented Jun 4, 2023

May be a minor thing but I think the construction (if-not (...) (do ... in the following:

    (if-not (has-value? boot/args "image-only") (do
      (print "\n/* " fname " */")
      ...))

here might be better expressed as either:

    (when (not (has-value? boot/args "image-only"))
      (print "\n/* " fname " */")
      ...)

or:

    (unless (has-value? boot/args "image-only")
      (print "\n/* " fname " */")
      ...)

The latter (using unless) is briefer, but I find it often takes me much longer to convince myself at a gut level that it's the "right thing" particularly when what's getting tested gets complicated.

(I've sometimes wanted a when-not, but I have been hesitant to suggest that kind of construction for potential bloat reasons (^^; )


Sorry about this remark coming after the merge -- I mentioned it at the fork earlier but I think it might not be so easy to find because of the force pushing may be?

@zevv
Copy link
Contributor Author

zevv commented Jun 5, 2023

Oh sorry @sogaiu, I saw your earlier remark and changed the code accordingly; this was on "my other machine" though and I forgot to push it.

@sogaiu
Copy link
Contributor

sogaiu commented Jun 5, 2023

Ah well, at least the code works :)

primo-ppcg added a commit to primo-ppcg/janet that referenced this pull request Jun 8, 2023
As suggested by @sogaiu

@zevv forget to push this change in a recent PR (janet-lang#1175 (comment)).

Incidentally, the affected lines were already reformatted in the current PR, via fmt/format-file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants