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

emacs: turn off optimizations, re-enable native-compilation #17319

Closed
wants to merge 2 commits into from

Conversation

cyrilarnould
Copy link

@cyrilarnould cyrilarnould commented May 24, 2023

Using the -O0 optimization level seems to address the issues that plague the previous two emacs releases. I tried -Og and -O1, but with those I was still getting problems.

Performance is not great, but still usable. As a quick test, I ran vhdl-beautify-buffer on a large VHDL file (one of the heavier emacs commands I know). A file which previously took 3.97 seconds to beautify now takes 23.91 seconds.

This might address issues #16413 #16375 and #16190, did not fully look into each of them though.

@Biswa96
Copy link
Member

Biswa96 commented May 25, 2023

Thank you for looking into this.

A file which previously took 3.97 seconds to beautify now takes 23.91 seconds.

Does this change fix any issue or add another issue?

@cyrilarnould
Copy link
Author

Does this change fix any issue or add another issue?

It should "fix" (obviously not a permanent solution) #16413 #16375 #16190. I mainly tested the UCRT64 build, where I didn't notice any of the problems mentioned in the open issues. I was able to install packages, CPU utilization seems normal and starting an async process (e.g. M-& dir) terminates properly. I also performed this last test on all builds as a quick verification.

As a tradeoff, it adds the issue of generally lower performance, which I measured with vhdl-beautify-buffer. I used https://github.com/OSVVM/AXI4/blob/main/Axi4/src/Axi4Manager.vhd as an example. You can run the command like this:

$ time emacs -Q /d/Git/OsvvmLibraries/AXI4/AXI4/src/Axi4Manager.vhd --batch --eval '(vhdl-beautify-buffer)'

real    0m22.085s
user    0m0.000s
sys     0m0.000s

Under emacs-28.2-2:

$ time emacs -Q /d/Git/OsvvmLibraries/AXI4/AXI4/src/Axi4Manager.vhd --batch --eval '(vhdl-beautify-buffer)'

real    0m5.067s
user    0m0.000s
sys     0m0.000s

I can open a new issue for posterity if you want once the PR is merged.

@raedrizqie
Copy link
Contributor

these patches are not needed anymore. it can be safely removed from the patch file..

  • the host=$cc_target should pass as long as we use --host=${MINGW_CHOST} when running configure
  • lld now supports -Wl,--heap

@@ -147,7 +147,7 @@
fi
cc_target=`$cc -v 2>&1 | sed -n 's/Target: //p'`
case "$cc_target" in
- *-*) host=$cc_target
+ *-*) host=$target_alias
;;
"") AC_MSG_ERROR([Impossible to obtain $cc compiler target.
Please explicitly provide --host.])
@@ -5765,8 +5765,8 @@
mingw32)
## Is it any better under MinGW64 to relocate emacs into higher addresses?
case "$canonical" in
- x86_64-*-*) LD_SWITCH_SYSTEM_TEMACS="-Wl,-stack,0x00800000 -Wl,-heap,0x00100000 -Wl,-image-base,0x400000000 -Wl,-entry,__start -Wl,-Map,./temacs.map" ;;
- *) LD_SWITCH_SYSTEM_TEMACS="-Wl,-stack,0x00800000 -Wl,-heap,0x00100000 -Wl,-image-base,0x01000000 -Wl,-entry,__start -Wl,-Map,./temacs.map" ;;
+ x86_64-*-*) LD_SWITCH_SYSTEM_TEMACS="-Wl,-stack,0x00800000 -Wl,-image-base,0x400000000 -Wl,-entry,__start -Wl,-Map,./temacs.map" ;;
+ *) LD_SWITCH_SYSTEM_TEMACS="-Wl,-stack,0x00800000 -Wl,-image-base,0x01000000 -Wl,-entry,__start -Wl,-Map,./temacs.map" ;;
esac
## If they want unexec, disable Windows ASLR for the Emacs binary
if test "$with_dumping" = "unexec"; then

@svraka
Copy link
Contributor

svraka commented May 26, 2023

I tried this build and it solves the sub-process related issues for me as well. Emacs seems to be a little bit more sluggish without the optimizations but very much usable. However, this could be just a placebo.

In #16190 there was some talk about – yet unspecified – ideas to properly fix the issue. In the meanwhile, @oscarfv do you think turning off optimizations is a good workaround?

@oscarfv
Copy link
Contributor

oscarfv commented May 26, 2023

Turning optimizations off is not acceptable, IMAO, performance is terrible. Popular packages such as lsp-emacs are slow enough with optimizations and native-comp, I'm pretty sure that with an unoptimized emacs they will be unusable.

OTOH, if -OO solves the problem, that's a very interesting fact, moreover when combined with the timeframe of the problem.

@cyrilarnould
Copy link
Author

cyrilarnould commented May 26, 2023

@oscarfv I agree, LSP will probably bring it to a crawl. I just thought it to be better than the current state of things since the package has been unusable for months with no solution in sight. OTOH, leaving the package broken creates more urgency for a true fix :)

@oscarfv
Copy link
Contributor

oscarfv commented May 26, 2023

Yes, the current state sucks big time.

A possible low-tech route for "fixing" the problem is to pinpoint the optimization that causes it. Since -Og still shows the problem, doing successive builds for a binary search with -O0 -opt1 -opt2... where each -opt is an optimization switch from those activated by -Og, with some patience the culprit would surface.

gcc optimization switches:

https://gcc.gnu.org/onlinedocs/gcc-13.1.0/gcc/Optimize-Options.html

@cyrilarnould
Copy link
Author

I actually wanted to try this, but compiling with -O0 and every single option up to -O2 enabled still seems to work ok. Performance was a little better, but the clang build failed, which is why I left it at -O0.

Not all optimizations are controlled directly by a flag. Only optimizations that have a flag are listed in this section.
Most optimizations are completely disabled at -O0 or if an -O level is not set on the command line, even if individual optimization flags are specified.

I believe the above would explain what I've seen. It's either an optimization not controllable via an individual flag thats causing the issue or one that is actively suppressed by -O0.

Delete superfluous clang patches
@cyrilarnould
Copy link
Author

I've managed to find the culprit: removing -Wp,-D_FORTIFY_SOURCE=2 from CFLAGS resolves the issues as well. This allows a return to -O2. I've reported the issue upstream:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63752

I've also deleted the patches @raedrizqie mentioned, the builds seem to work fine without them.

@oscarfv
Copy link
Contributor

oscarfv commented May 27, 2023

This is fantastic news, thank you @cyrilarnould !

Would you please create a new PR with just the removal of _FORTIFY_SOURCE? Having two successive commits with the second partly reverting the first is undesirable.

@cyrilarnould
Copy link
Author

Can do, although I'm worried about the ucrt64/mingw64 builds taking this long... Is there a way to look into the logs to see where they're hanging exactly?

In the second PR, should I also remove the clang patches or keep them?

@svraka
Copy link
Contributor

svraka commented May 27, 2023

I've managed to find the culprit: removing -Wp,-D_FORTIFY_SOURCE=2 from CFLAGS resolves the issues as well. This allows a return to -O2.

That would also explain how the GNU build of Emacs 29 works fine while building it from MSYS doesn't. They built that with optimizations but without extra flags: "--with-modules --without-dbus --with-native-compilation=aot --without-compress-install --with-tree-sitter CFLAGS=-O2".

@oscarfv
Copy link
Contributor

oscarfv commented May 27, 2023

Can do, although I'm worried about the ucrt64/mingw64 builds taking this long... Is there a way to look into the logs to see where they're hanging exactly?

In the second PR, should I also remove the clang patches or keep them?

The long compilation time is, most likely, because of you re-enabling nativecomp. @Biswa96 disabled it because gcc 13.1 requires a very long time to compile the .el files to native .elc. With your initial change to -O0 native comp was fast again.

I suggest that you submit the removal of the clang patches on a different PR. Thank you.

@cyrilarnould
Copy link
Author

I see, thanks for the explanation. I'll leave the clang patches to @raedrizqie then :)

@cyrilarnould
Copy link
Author

@oscarfv BTW, upstream is understandably passing the ball along to MSYS2. I've noticed that _FORTIFY_SOURCE=2 has been added rather recently:

https://github.com/msys2/MSYS2-packages/commits/master/pacman/makepkg_mingw.conf

Should I open an issue in the MSYS2-packages or MINGW-packages repository referencing this emacs build problem for further investigations?

@oscarfv
Copy link
Contributor

oscarfv commented May 27, 2023

It is a good idea even if the issue remains inactive: it registers the existence of the problem.

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.

None yet

5 participants