ocaml / dune

A composable build system for OCaml.
https://dune.build/
MIT License
1.6k stars 397 forks source link

Very slow emacs compilation buffer updates #8242

Open samoht opened 1 year ago

samoht commented 1 year ago

I don't know how to report this correctly, but something is interfering with the emacs buffer refresh rate in the recent version of Dune (it was okay-ish in 3.7 but is now super slow in 3.9).

How to reproduce:

How to fix:

I suspect the right fix is always to use --display=quiet in the emacs plugin, but there's probably a reason for it not to be the default.

Alizter commented 1 year ago

We have an INSIDE_EMACS option that was originally intended to set the display to quiet. I think that might have been broken when reorganizing the console code.

At the moment we switch to the unthreaded console when no progress line is being displayed. This has caused performance problems in the past which were fixed by choosing the threaded console. The perf issues could just be the status line however which shouldn't be shown in emacs if I've read correctly.

I wonder if setting/unsetting INSIDE_EMACS variable has any effect now.

nojb commented 1 year ago

I wonder if setting/unsetting INSIDE_EMACS variable has any effect now.

Just to make sure we are on the same page: INSIDE_EMACS is set by Emacs, not Dune (see https://www.gnu.org/software/emacs/manual/html_node/emacs/Interactive-Shell.html). Dune is just supposed to honour this variable to make any adjustments needed to run under Emacs.

fpottier commented 1 year ago

I observe the same problem after upgrading from dune 3.6 to dune 3.10. I am using Emacs 27.2 under MacOS.

Alizter commented 1 year ago

I took a quick look and the logic works like this. We only display the progress bar if we are in a tty. As an exception if INSIDE_EMACS is also set then we display the progress bar even if not in a tty (just like in an emacs terminal).

I've checked 3.6 and main and this behaviour is still consistent. Behind the scenes this logic was factored so I was originally wondering if there was a mistake.

I'm now thinking that this might be an issue with the threaded vs unthreaded console. IIRC we default to a "dumb" terminal for non-tty terminals therefore no threading for the progress bar.

@rgrinberg do you think this might be the culprit?

rgrinberg commented 1 year ago

Defo worth trying threading vs non threading when running dune in emacs. @nojb do you mind experimenting with this? (I don't any other maintainer that uses emacs).

nojb commented 1 year ago

@nojb do you mind experimenting with this? (I don't any other maintainer that uses emacs).

OK, I will give it a try (but first I need to reproduce the issue).

nojb commented 1 year ago

OK, I will give it a try (but first I need to reproduce the issue).

I am unable to reproduce the issue under Linux (using terminal Emacs) or Windows (using GUI Emacs). I invoked Dune using M-x compile, then dune build. I will try to get my hands on a macOS machine later to give it a try there.

I observe the same problem after upgrading from dune 3.6 to dune 3.10. I am using Emacs 27.2 under MacOS.

How are you invoking Dune?

camlspotter commented 11 months ago

I tried

with dune.3.10.0 in the following environments:

The slowness is just like @samoht has described.

BTW, main branch commit aff8994c1302cb3f9a9af54763867e8f8bfbf043 has no such slowness. The problem is already fixed?

Alizter commented 11 months ago

@camlspotter If you could bisect that would be very useful.

camlspotter commented 11 months ago

@Alizter The following commit fixes the issue. I hope it helps:

be9aa2820 * feature: use posix_spawn on macos (#8090)

It may match with my observation: the slowness of 3.10.0 was only observed in Mac OS.

Alizter commented 11 months ago

@camlspotter Thank you very much for the match!

@rgrinberg does this sound about right? Which version was this issue introduced? Is it perhaps worth some backports?

rgrinberg commented 11 months ago

Nope, that commit doesn't seem relevant to running dune in emacs to me.

Alizter commented 11 months ago

@rgrinberg Should we be running dune in a single threaded mode when inside emacs? If that commit really did have an effect, then it might mean at some point we accidentally started using multithreaded mode hence the slowdown on emacs and speedup when switching to posix_spawn.

rgrinberg commented 11 months ago

That commit did enable multi threading for macos. However, it still has no relationship to emacs. Maybe it was the terminal IO being very slow on Emacs so doing it in a background thread fixed it?

@samoht can you confirm if this is still repro?

samoht commented 11 months ago

~I confirm I cannot see the issue anymore in main.~ I can't reproduce the issue anymore with my (updated) emacs config (GNU Emacs 29.1).

Alizter commented 11 months ago

@rgrinberg INSIDE_EMACS is (supposed to be) forcing single threading IIRC. Albeit in a very weird and indirect manner. The logic here is quite hard to follow:

  let adapt_display config ~output_is_a_tty =
    (* Progress isn't meaningful if inside a terminal (or emacs), so disable it if
       the output is getting piped to a file or something. *)
    let config =
      match config.display with
      | Simple { verbosity; _ }
        when (not output_is_a_tty) && not Execution_env.inside_emacs ->
        { config with display = Simple { verbosity; status_line = false } }
      | _ -> config
    in
    (* Similarly, terminal clearing is meaningless if stderr doesn't support ANSI
       codes, so revert-back to Preserve in that case *)
    if config.terminal_persistence = Clear_on_rebuild && not output_is_a_tty
    then { config with terminal_persistence = Terminal_persistence.Preserve }
    else config
  ;;

So it looks like the comment says "disable progress if inside emacs" but the logic does the opposite which is "not being a tty" and "not being inside emacs" then disable progress.

Later down the line when we choose a console back end, we do the following:

let console_backend = function
  | Tui -> Dune_tui.backend ()
  | Simple { status_line; _ } ->
    (match status_line with
     | false ->
       Dune_util.Terminal_signals.unblock ();
       Dune_console.Backend.dumb
     | true ->
       (match Config.(get threaded_console) with
        | `Enabled -> Dune_threaded_console.progress ()
        | `Disabled ->
          Dune_util.Terminal_signals.unblock ();
          Dune_console.Backend.progress))
;;

So the status_line is incorrectly set on emacs/macos which means we fallback on the value of get threaded_console which on Darwin is set to true. Therefore emacs is running multithreaded with a progress indicator.

The correct fix here seems to be to make sure we are disabling the progress bar when INSIDE_EMACS is set. That should in turn cause our terminal backend to fallback on the dumb terminal which is non-threaded.

Does that make sense?

samoht commented 11 months ago

Ok, I'm confused now.

Something has changed in my configuration, but dune now outputs a blue [...] output instead of the slowly refreshing progress report. It might be related to an upgrade to a new emacs version (29.1).

Not totally sure what's going on

Alizter commented 11 months ago

@samoht That sounds like emacs is truncating long lines. If the progress indicators are not being cleared correctly then it could be that emacs thinks this is one really long line.

Alizter commented 11 months ago

Can somebody try https://github.com/ocaml/dune/pull/8809 and see if it makes things better. I revert the posix_spawn so we can get the bad behaviour and add a commit that makes dune detect and adapt to emacs properly. Let me know if it makes a difference wtih and wtihout the fix commit.

Edit: actually a better fix would be #8812. If you test this patch, you should be able to configure the rate at which dune refreshes (in a backdoor fashion) by setting DUNE_CONFIG__THREADED_CONSOLE_REFRESH_RATE to a frame rate. Dune is by default 60 which might cause issues in emacs due to the frequent updates. Therefore in that PR I detect INSIDE_EMACS and default to 15 instead, which might be a more suitable value.

Regarding the issue with the status line appearing many times, I believe that this is due to status line clearing not working properly when text is wrapped, which is probably common inside a narrow emacs buffer. The fix for this is in #8619 but that's waiting for some other patch in order to get windows compat.