From 88d7dd5505e78f22011f63c5d6d67f980a5fed5e Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Fri, 14 Oct 2022 17:29:58 -0700 Subject: [PATCH 1/4] Enable link-time optimization for Vim in MacVim CI builds From local profiling, enabling LTO for Vim gives a small but measurable improvement to performance. One test that I did was to open a really large Markdown file with vim-markdown (which usually chokes at large files) installed, and measure how long that takes. With LTO turned on, usually it gives at least 6-10% performance boost, which seems significant enough to justify turning it on as we essentially get the improvement for free (I didn't see similar boosts in other benhcmarking I did though, so it depends). Slight caveat is that the binary size sees a small increase (presumably due to inlining) but it's not too much. It takes more time to build with this turned on though, so only do this in CI, for the publish builds (we don't do this for the other runs in the matrix so those runs can finish faster to provide timely feedbacks). This doesn't change the compilation/linking options for MacVim binary itself as that doesn't seem to be where performance caps are. --- .github/workflows/ci-macvim.yaml | 6 ++++++ ci/config.mk.lto.sed | 3 +++ 2 files changed, 9 insertions(+) create mode 100644 ci/config.mk.lto.sed diff --git a/.github/workflows/ci-macvim.yaml b/.github/workflows/ci-macvim.yaml index 3f59a699ae..f8108b521b 100644 --- a/.github/workflows/ci-macvim.yaml +++ b/.github/workflows/ci-macvim.yaml @@ -137,6 +137,12 @@ jobs: sed -i.bak -f ci/config.mk.clang-12.sed src/auto/config.mk fi + if ${{ matrix.publish == true }}; then + # Only do link-time optimizations for publish builds, so the other + # ones can still finish quickly to give quick feedbacks. + sed -i.bak -f ci/config.mk.lto.sed src/auto/config.mk + fi + - name: Modify configure result if: matrix.publish run: | diff --git a/ci/config.mk.lto.sed b/ci/config.mk.lto.sed new file mode 100644 index 0000000000..8057fa6366 --- /dev/null +++ b/ci/config.mk.lto.sed @@ -0,0 +1,3 @@ +# Add link-time optimization for even better performance +/^CFLAGS[[:blank:]]*=/s/$/ -flto/ +/^LDFLAGS[[:blank:]]*=/s/$/ -flto/ From 491087b707f0cc20e7d9534087c6c347bbdb20ba Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sat, 15 Oct 2022 00:37:54 -0700 Subject: [PATCH 2/4] Fix version.o so it doesn't invalidates other binaries every time it's used --- src/Makefile | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Makefile b/src/Makefile index b7d773d541..6d1de2cf78 100644 --- a/src/Makefile +++ b/src/Makefile @@ -2067,7 +2067,6 @@ CCC = $(CCC_NF) $(ALL_CFLAGS) # Link the target for normal use or debugging. # A shell script is used to try linking without unnecessary libraries. $(VIMTARGET): auto/config.mk objects $(OBJ) version.c version.h - $(CCC) version.c -o objects/version.o @$(BUILD_DATE_MSG) @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(VIMTARGET) $(OBJ) $(ALL_LIBS)" \ @@ -2272,28 +2271,24 @@ testclean: # Unittests # It's build just like Vim to satisfy all dependencies. $(JSON_TEST_TARGET): auto/config.mk objects $(JSON_TEST_OBJ) - $(CCC) version.c -o objects/version.o @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(JSON_TEST_TARGET) $(JSON_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ sh $(srcdir)/link.sh $(KWORD_TEST_TARGET): auto/config.mk objects $(KWORD_TEST_OBJ) - $(CCC) version.c -o objects/version.o @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(KWORD_TEST_TARGET) $(KWORD_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ sh $(srcdir)/link.sh $(MEMFILE_TEST_TARGET): auto/config.mk objects $(MEMFILE_TEST_OBJ) - $(CCC) version.c -o objects/version.o @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(MEMFILE_TEST_TARGET) $(MEMFILE_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ sh $(srcdir)/link.sh $(MESSAGE_TEST_TARGET): auto/config.mk objects $(MESSAGE_TEST_OBJ) - $(CCC) version.c -o objects/version.o @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(MESSAGE_TEST_TARGET) $(MESSAGE_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ @@ -3509,6 +3504,9 @@ objects/usercmd.o: usercmd.c objects/userfunc.o: userfunc.c $(CCC) -o $@ userfunc.c +objects/version.o: version.c + $(CCC) -o $@ version.c + objects/vim9cmds.o: vim9cmds.c $(CCC) -o $@ vim9cmds.c From fe71912b6117244616443b03a445481348622b2f Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sat, 15 Oct 2022 13:00:01 -0700 Subject: [PATCH 3/4] Make Vim and unit test targets not blindly depend on objects folder They already depend on the relevant .o files, so no need to depend on the whole folder. Depending on the objects folder make them stomp each other all the time. --- src/Makefile | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Makefile b/src/Makefile index 6d1de2cf78..6ff5fdc094 100644 --- a/src/Makefile +++ b/src/Makefile @@ -2066,7 +2066,7 @@ CCC = $(CCC_NF) $(ALL_CFLAGS) # Link the target for normal use or debugging. # A shell script is used to try linking without unnecessary libraries. -$(VIMTARGET): auto/config.mk objects $(OBJ) version.c version.h +$(VIMTARGET): auto/config.mk $(OBJ) version.c version.h @$(BUILD_DATE_MSG) @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(VIMTARGET) $(OBJ) $(ALL_LIBS)" \ @@ -2270,25 +2270,25 @@ testclean: # Unittests # It's build just like Vim to satisfy all dependencies. -$(JSON_TEST_TARGET): auto/config.mk objects $(JSON_TEST_OBJ) +$(JSON_TEST_TARGET): auto/config.mk $(JSON_TEST_OBJ) @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(JSON_TEST_TARGET) $(JSON_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ sh $(srcdir)/link.sh -$(KWORD_TEST_TARGET): auto/config.mk objects $(KWORD_TEST_OBJ) +$(KWORD_TEST_TARGET): auto/config.mk $(KWORD_TEST_OBJ) @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(KWORD_TEST_TARGET) $(KWORD_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ sh $(srcdir)/link.sh -$(MEMFILE_TEST_TARGET): auto/config.mk objects $(MEMFILE_TEST_OBJ) +$(MEMFILE_TEST_TARGET): auto/config.mk $(MEMFILE_TEST_OBJ) @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(MEMFILE_TEST_TARGET) $(MEMFILE_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ sh $(srcdir)/link.sh -$(MESSAGE_TEST_TARGET): auto/config.mk objects $(MESSAGE_TEST_OBJ) +$(MESSAGE_TEST_TARGET): auto/config.mk $(MESSAGE_TEST_OBJ) @LINK="$(PURIFY) $(SHRPENV) $(CClink) $(ALL_LIB_DIRS) $(LDFLAGS) \ -o $(MESSAGE_TEST_TARGET) $(MESSAGE_TEST_OBJ) $(ALL_LIBS)" \ MAKE="$(MAKE)" LINK_AS_NEEDED=$(LINK_AS_NEEDED) \ From 26c39df318a822e896b379e8e2ca8f50efb99be4 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Sat, 15 Oct 2022 13:06:28 -0700 Subject: [PATCH 4/4] Separate out the unit test targets from actual test --- .github/workflows/ci-macvim.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/ci-macvim.yaml b/.github/workflows/ci-macvim.yaml index f8108b521b..cdd9a76de1 100644 --- a/.github/workflows/ci-macvim.yaml +++ b/.github/workflows/ci-macvim.yaml @@ -236,6 +236,11 @@ jobs: check_arch "${VIM_BIN}" check_arch "${MACVIM_BIN}" + # Build the unit test binaries first so if they fail, it'll be at a different step from the actual test. Also, + # with link-time-optimizations, this help prevent them from using up the timeout in the tests. + - name: Build test binaries + run: make -C src unittesttargets + - name: Test timeout-minutes: 20 run: make test