-
Notifications
You must be signed in to change notification settings - Fork 409
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
feature: use posix_spawn on macos #8090
Conversation
cefe9a9
to
02c5b5f
Compare
@anmonteiro @jchavarri can you check if you can reproduce the bug on this PR? |
I can't reproduce the hang on macOS anymore with this PR. |
I'm running it through the full set of packages too: nix-ocaml/nix-overlays#926 |
This might not be that risky, the CI run succeeded: https://github.com/nix-ocaml/nix-overlays/actions/runs/5466251548/jobs/9950895919 |
Can't repro the original issue with this PR either. |
This PR has some advantages from a performance standpoint. So it might be good to report if you notice anything. I know @anmonteiro did some benchmarks on melange. |
I can see some significant improvements yes. From 8% to 25% faster with this branch, it seems to affect particularly Melange builds (that's where I see the 25% improvement). Note that the mac CI nodes we use have 12 cores, I imagine that the more capacity there is to parallelize, the larger the impact on build times? |
@rgrinberg what's missing to get this one over the line? |
If you could read over and familiarize yourself with the C to give a formal review that would be a great start. |
} | ||
|
||
for (int fd = 0; fd < 3; fd++) { | ||
int tmp_fd = tmp_fds[fd] = safe_dup(info.std_fds[fd]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth it to copy the upstream concern about resetting close-on-exec, and think about whether we'd want to solve it here:
https://github.com/janestreet/spawn/pull/47/files#r930889893
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we'll do it upstream. Note that I've talked to the maintainers and we agreed that this isn't an issue for dune because we don't allow anywhere else in the code to use fork.
02c5b5f
to
601a25f
Compare
Signed-off-by: Rudi Grinberg <me@rgrinberg.com> <!-- ps-id: e726a301-bd20-460c-a5f1-a65730739bba -->
601a25f
to
9f37ca8
Compare
@anmonteiro could you give this another test please? I re-enabled threads on macos since they longer should be a problem and should give us an additional perf benefit. |
Running this through all the packages here: nix-ocaml/nix-overlays#1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
All macOS packages build on nix-overlays. They were previously broken with fork
Just reporting some build times i measured after this PR was merged in a quite large Melange build on our codebase, using my macbook pro (16 cores). Using an older version of dune (e1f3e19):
Using latest dune (758e370):
Thanks @rgrinberg ! |
Here are the results of compiling the same large Melange codebase on the M2 MacBook Air on the latest dune (df4399b)
vs older version of dune (e1f3e19)
@rgrinberg 👏 👏 👏 |
CHANGES: - Modules that were declared in `(modules_without_implementation)`, `(private_modules)` or `(virtual_modules)` but not declared in `(modules)` will raise an error. (ocaml/dune#7674, @Alizter) - `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997, @Alizter)- Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg) - Experimental: Added a `$ dune monitor` command that can connect to a running `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152, @Alizter) - No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo) - The `progress` RPC procedure now has an extra field for the `In_progress` constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter) - Add a `--preview` flag to `dune fmt` which causes it to print out the changes it would make without applying them (ocaml/dune#8289, @gridbugs) - Introduce `(source_trees ..)` to the install stanza to allow installing entire source trees. (ocaml/dune#8349, @rgrinberg) - Deprecate install destination paths beginning with ".." to prevent packages escaping their designated installation directories. (ocaml/dune#8350, @gridbugs) - Stop signing source files with substitutions. Sign only binaries instead (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro) - Add `--stop-on-first-error` option to `dune build` which will terminate the build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)- Dune now displays the number of errors when waiting for changes in watch mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter) - Add `with_prefix` keyword for changing the prefix of the destination of installed files matched by globs. (ocaml/dune#8416, @gridbugs) - Added experimental `--display tui` option for Dune that opens an interactive Terminal User Interface (TUI) when Dune is running. Press '?' to open up a help screen when running for more information. (ocaml/dune#8429, @Alizter and @rgrinberg) - Add a `warnings` field to `dune-project` files as a unified mechanism to enable or disable dune warnings (@rgrinberg, 8448) - `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere in the command line, so things like `dune exec time %{bin:program}` now work. (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV) - RPC message styles are now serialised meaning that RPC diagnostics keep their Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)- Ignore internal promote rules when `--ignore-promoted-rules` is set (ocaml/dune#8518, fix ocaml/dune#8417, @rgrinberg) - Truncate output from actions that produce too much output (@tov, ocaml/dune#8351) - Allow libraries to shadow OCaml builtin libraries. Previously, builtin libraries would always take precedence. (@rgrinberg, ocaml/dune#8558) - Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611, @rgrinberg) - `dune utop` no longer links `utop` in "custom" mode, which should make this command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb) - Ensure that package names in `dune-project` are valid opam package names. (ocaml/dune#8331, @emillon) - dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon) - Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293, @emillon)
CHANGES: - `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997, @Alizter) - Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg) - Experimental: Added a `$ dune monitor` command that can connect to a running `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152, @Alizter) - The `progress` RPC procedure now has an extra field for the `In_progress` constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter) - Add a `--preview` flag to `dune fmt` which causes it to print out the changes it would make without applying them (ocaml/dune#8289, @gridbugs) - Introduce `(source_trees ..)` to the install stanza to allow installing entire source trees. (ocaml/dune#8349, @rgrinberg) - Add `--stop-on-first-error` option to `dune build` which will terminate the build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter) - Dune now displays the number of errors when waiting for changes in watch mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter) - Add `with_prefix` keyword for changing the prefix of the destination of installed files matched by globs. (ocaml/dune#8416, @gridbugs) - Added experimental `--display tui` option for Dune that opens an interactive Terminal User Interface (TUI) when Dune is running. Press '?' to open up a help screen when running for more information. (ocaml/dune#8429, @Alizter and @rgrinberg) - Add a `warnings` field to `dune-project` files as a unified mechanism to enable or disable dune warnings (@rgrinberg, 8448) - `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere in the command line, so things like `dune exec time %{bin:program}` now work. (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV) - Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon) - Add a new alias `@doc-json` to build odoc documentation in JSON format. This output can be consumed by external tools. (ocaml/dune#8178, @emillon) - Modules that were declared in `(modules_without_implementation)`, `(private_modules)` or `(virtual_modules)` but not declared in `(modules)` will raise an error. (ocaml/dune#7674, @Alizter) - No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo) - Deprecate install destination paths beginning with ".." to prevent packages escaping their designated installation directories. (ocaml/dune#8350, @gridbugs) - RPC message styles are now serialised meaning that RPC diagnostics keep their Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter) - Truncate output from actions that produce too much output (@tov, ocaml/dune#8351) - Allow libraries to shadow OCaml builtin libraries. Previously, builtin libraries would always take precedence. (@rgrinberg, ocaml/dune#8558) - Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611, @rgrinberg) - `dune utop` no longer links `utop` in "custom" mode, which should make this command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb) - Ensure that package names in `dune-project` are valid opam package names. (ocaml/dune#8331, @emillon) - init: check that module names are valid (ocaml/dune#8644, fixes ocaml/dune#8252, @emillon) - dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon) - Stop signing source files with substitutions. Sign only binaries instead (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro) - Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293, @emillon)
This reverts commit be9aa28.
@rgrinberg This does not seem right. Let me try fixing this and see if it builds then. |
@@ -13,6 +13,39 @@ CAMLextern int caml_convert_signal_number(int); | |||
|
|||
#if defined(__APPLE__) | |||
|
|||
# if defined(__MAC_OS_X_VERSION_MAX_ALLOWED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgrinberg What this macro is supposed to do specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is wrong. The check should be for 10.5 because we need pthread_chdir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is what fails for me on 10.6 ppc, which will be similar to 10.5 in this regard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two more issues:
environ
define is wrong for macOS.- There seems to be incoherency in the code, at first
vfork fork
is defined generally, then below the same only for macOS > 12000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if with these fixes it works. Will update in few min.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks. Note that we can't use vfork
on macos. So it must be a leftover from the old code when we did.
@rgrinberg After these 39703e0 I got here:
How to get a proper log to see what fails exactly? |
What kind of log are you looking for? There is no more information for this error. Try running this executable maunally |
Well, just in case.
This is literally a symlink to an empty file. So for w/e reason it is not written correctly, and build system is unaware of that. P. S. Could you please take a look at my changes and say what should be done re |
Okay, that's the source of the error then. What command were you using to build dune and then to generate this error?
Sure, where are your changes? Regarding posix_spawn vs fork vs vfork:
So on macos, we should only use fork if our posix_spawn is good enough and pthread_chdir is available (I believe anything above 10.5, but I could be wrong). |
@rgrinberg Changes are here: 39703e0 And
I build from the port: https://github.com/macports/macports-ports/blob/master/ocaml/ocaml-dune/Portfile |
@rgrinberg Changing the code to:
I still get P. S. This new error does not need to be spawn-related. The last version I can say built and worked for sure was 3.7.0 and master branch as of 2022.11.18. Everything in between that and 3.11.0 has not been tried, at least yet. UPD. I am pretty sure now that |
@barracuda156 Could you try 3.10 to make sure it is this PR that is causing problems. |
@Alizter It looks like we have fixed
|
Try disabling fast copying completely with |
Sorry for a silly question, where do I pass this? Configure does not seem to take this as an arg. |
It's an environment variable, so you just need it to set when running dune. E.g. |
Unfortunately, no effect:
But perhaps we patched out both copy_file and posix_spawn issues. This is something else, it seems. |
@barracuda156 I think that's a typo, it should be |
@Alizter @rgrinberg So yes, What are the conclusions? Could we fix it properly rather than just disabling? UPD. I still need to verify if resulting |
I gonna return to this in detail tomorrow, need to leave now, but preliminary results are problematic: Whether built with my patch to spawn or without it,
(This patch https://github.com/ocaml/dune/pull/8942/files is required, without it
Apparently while |
I think that disabling I should say however that this isn't something we will probably do any time soon since we currently have other priorities in Dune and not that many developers. However if you feel like creating a patch yourself, we can try to review it when we have the time. Since OCaml isn't looking to officially support powerpc we have even less of a reason to. c.f. ocaml/ocaml#11216 You can also subsume my patch for the copyfile C stubs and add an appropriate comment detailing why its needed. |
@Alizter OCaml does support all 32-bit platforms including powerpc in bytecode compiler (this applies to OCaml 5). That was the stance of upstream at least rather recently. Native was broken anyway, so nothing changes. The thread you refer to was about native compiler, which was broken and remained so, and then in OCaml 5 support for |
CHANGES: - `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997, @Alizter) - Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg) - Experimental: Added a `$ dune monitor` command that can connect to a running `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152, @Alizter) - The `progress` RPC procedure now has an extra field for the `In_progress` constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter) - Add a `--preview` flag to `dune fmt` which causes it to print out the changes it would make without applying them (ocaml/dune#8289, @gridbugs) - Introduce `(source_trees ..)` to the install stanza to allow installing entire source trees. (ocaml/dune#8349, @rgrinberg) - Add `--stop-on-first-error` option to `dune build` which will terminate the build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter) - Dune now displays the number of errors when waiting for changes in watch mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter) - Add `with_prefix` keyword for changing the prefix of the destination of installed files matched by globs. (ocaml/dune#8416, @gridbugs) - Added experimental `--display tui` option for Dune that opens an interactive Terminal User Interface (TUI) when Dune is running. Press '?' to open up a help screen when running for more information. (ocaml/dune#8429, @Alizter and @rgrinberg) - Add a `warnings` field to `dune-project` files as a unified mechanism to enable or disable dune warnings (@rgrinberg, 8448) - `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere in the command line, so things like `dune exec time %{bin:program}` now work. (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV) - Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon) - Add a new alias `@doc-json` to build odoc documentation in JSON format. This output can be consumed by external tools. (ocaml/dune#8178, @emillon) - Modules that were declared in `(modules_without_implementation)`, `(private_modules)` or `(virtual_modules)` but not declared in `(modules)` will raise an error. (ocaml/dune#7674, @Alizter) - No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo) - Deprecate install destination paths beginning with ".." to prevent packages escaping their designated installation directories. (ocaml/dune#8350, @gridbugs) - RPC message styles are now serialised meaning that RPC diagnostics keep their Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter) - Truncate output from actions that produce too much output (@tov, ocaml/dune#8351) - Allow libraries to shadow OCaml builtin libraries. Previously, builtin libraries would always take precedence. (@rgrinberg, ocaml/dune#8558) - Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611, @rgrinberg) - `dune utop` no longer links `utop` in "custom" mode, which should make this command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb) - Ensure that package names in `dune-project` are valid opam package names. (ocaml/dune#8331, @emillon) - init: check that module names are valid (ocaml/dune#8644, fixes ocaml/dune#8252, @emillon) - dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon) - Stop signing source files with substitutions. Sign only binaries instead (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro) - Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293, @emillon)
Signed-off-by: Rudi Grinberg me@rgrinberg.com