forked from git/git
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Switch Git for Windows to using mimalloc instead of nedmalloc #4013
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdf119d
to
0d768a6
Compare
derrickstolee
approved these changes
Sep 8, 2022
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 have a few questions that might clarify some of my understanding, but overall I think this is ready.
jeffhostetler
approved these changes
Sep 8, 2022
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.
Looks good to me!
The mingw-w64 GCC seems to link implicitly to libwinpthread, which does implement a pthread emulation (that is more complete than Git's). Let's keep preferring Git's. To avoid linker errors where it thinks that the `pthread_self` and the `pthread_create` symbols are defined twice, let's give our version a `win32_` prefix, just like we already do for `pthread_join()`. Signed-off-by: Johannes Schindelin <[email protected]>
We are about to vendor in `mimalloc`'s source code which we will want to include `git-compat-util.h` after defining that constant. Signed-off-by: Johannes Schindelin <[email protected]>
This commit imports mimalloc's source code as per v2.0.6, fetched from the tag at https://github.com/microsoft/mimalloc. The .c files are from the src/ subdirectory, and the .h files from the include/ subdirectory. We will subsequently modify the source code to accommodate building within Git's context. Since we plan on using the `mi_*()` family of functions, we skip the C++-specific source code, some POSIX compliant functions to interact with mimalloc, and the code that wants to support auto-magic overriding of the `malloc()` function (mimalloc-new-delete.h, alloc-posix.c, mimalloc-override.h, alloc-override.c, alloc-override-osx.c, alloc-override-win.c and static.c). To appease the `check-whitespace` job of Git's Continuous Integration, this commit was washed one time via `git rebase --whitespace=fix`. Signed-off-by: Johannes Schindelin <[email protected]>
We want to compile mimalloc's source code as part of Git, rather than requiring the code to be built as an external library: mimalloc uses a CMake-based build, which is not necessarily easy to integrate into the flavors of Git for Windows (which will be the main benefitting port). Signed-off-by: Johannes Schindelin <[email protected]>
This format is not supported by MSVC runtimes targeted by the MINGW toolchain. Signed-off-by: Johannes Schindelin <[email protected]>
Instead, load the `GetProcessMemoryInfo()` function dynamically. When needed. If needed. This is necessary because the start-up cost of Git processes spent on loading dynamic libraries is non-negligible. Signed-off-by: Johannes Schindelin <[email protected]>
Instead, load the `BCryptGenRandom()` function dynamically. When needed. If needed. This is necessary because the start-up cost of Git processes spent on loading dynamic libraries is non-negligible. Signed-off-by: Johannes Schindelin <[email protected]>
By defining `USE_MIMALLOC`, Git can now be compiled with that nicely-fast and small allocator. Note that we have to disable a couple `DEVELOPER` options to build mimalloc's source code, as it makes heavy use of declarations after statements, among other things that disagree with Git's conventions. For example, the `-Wno-array-bounds` flag is needed because in `-O2` builds, trying to call `NtCurrentTeb()` (which `_mi_thread_id()` does on Windows) causes the bogus warning about a system header, likely related to https://sourceforge.net/p/mingw-w64/mailman/message/37674519/ and to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578: C:/git-sdk-64-minimal/mingw64/include/psdk_inc/intrin-impl.h:838:1: error: array subscript 0 is outside array bounds of 'long long unsigned int[0]' [-Werror=array-bounds] 838 | __buildreadseg(__readgsqword, unsigned __int64, "gs", "q") | ^~~~~~~~~~~~~~ Also: The `mimalloc` library uses C11-style atomics, therefore we must require that standard when compiling with GCC if we want to use `mimalloc` (instead of requiring "only" C99). This is what we do in the CMake definition already, therefore this commit does not need to touch `contrib/buildsystems/`. Signed-off-by: Johannes Schindelin <[email protected]>
Setting `MIMALLOC_SHOW_STATS` to ask mimalloc to print out something after the process is done is the easiest way to verify that a mimalloc-enabled Git is running. So it better work and not try to write to a Win32 Console when it got a regular file handle instead or, as is the case in Git for Windows' regular Git Bash window, an emulated pseudo terminal. Signed-off-by: Johannes Schindelin <[email protected]>
Thorough benchmarking with repacking a subset of linux.git (the commit history reachable from 93a6fef ([PATCH] fix the SYSCTL=n compilation, 2007-02-28), to be precise) suggest that this allocator is on par, in multi-threaded situations maybe even better than nedmalloc: `git repack -adfq` with mimalloc, 8 threads: 31.166991900 27.576763800 28.712311000 27.373859000 27.163141900 `git repack -adfq` with nedmalloc, 8 threads: 31.915032900 27.149883100 28.244933700 27.240188800 28.580849500 In a different test using GitHub Actions build agents (probably single-threaded, a core-strength of nedmalloc)): `git repack -q -d -l -A --unpack-unreachable=2.weeks.ago` with mimalloc: 943.426 978.500 939.709 959.811 954.605 `git repack -q -d -l -A --unpack-unreachable=2.weeks.ago` with nedmalloc: 995.383 952.179 943.253 963.043 980.468 While these measurements were not executed with complete scientific rigor, as no hardware was set aside specifically for these benchmarks, it shows that mimalloc and nedmalloc perform almost the same, nedmalloc with a bit higher variance and also slightly higher average (further testing suggests that nedmalloc performs worse in multi-threaded situations than in single-threaded ones). In short: mimalloc seems to be slightly better suited for our purposes than nedmalloc. Seeing that mimalloc is developed actively, while nedmalloc ceased to see any updates in eight years, let's use mimalloc on Windows instead. Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci
pushed a commit
that referenced
this pull request
Sep 8, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Sep 8, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Sep 8, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
that referenced
this pull request
Sep 8, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Sep 8, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 21, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 21, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 21, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 21, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 26, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 26, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 28, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 28, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Dec 28, 2022
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Jan 2, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Jan 5, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
that referenced
this pull request
Jan 6, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
that referenced
this pull request
Jan 6, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
git-for-windows-ci
pushed a commit
that referenced
this pull request
Jan 7, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
that referenced
this pull request
Jan 9, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
that referenced
this pull request
Jan 9, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
that referenced
this pull request
Jan 9, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
to dscho/git
that referenced
this pull request
Jan 11, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
dscho
added a commit
to dscho/git
that referenced
this pull request
Jan 11, 2023
In preparation for vendoring in a newer version of mimalloc, let's drop the current version of the topic. This reverts commit 8f9a61f, reversing changes made to 145a228. Signed-off-by: Johannes Schindelin <[email protected]>
derrickstolee
pushed a commit
that referenced
this pull request
Jan 17, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
derrickstolee
pushed a commit
to microsoft/git
that referenced
this pull request
Feb 14, 2023
Switch Git for Windows to using mimalloc instead of nedmalloc
6 tasks
dscho
added a commit
to dscho/git
that referenced
this pull request
Aug 30, 2023
Just like in the regular GCC (MINGW) build, let's do the same in the CMake (Visual C) build. This is a long-overdue companion patch to git-for-windows#4013 Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
to dscho/git
that referenced
this pull request
Dec 19, 2023
Just like in the regular GCC (MINGW) build, let's do the same in the CMake (Visual C) build. This is a long-overdue companion patch to git-for-windows#4013 Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
to dscho/git
that referenced
this pull request
Dec 21, 2023
Just like in the regular GCC (MINGW) build, let's do the same in the CMake (Visual C) build. This is a long-overdue companion patch to git-for-windows#4013 Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
to dscho/git
that referenced
this pull request
Dec 21, 2023
Just like in the regular GCC (MINGW) build, let's do the same in the CMake (Visual C) build. This is a long-overdue companion patch to git-for-windows#4013 Signed-off-by: Johannes Schindelin <[email protected]>
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As per the very author of
nedmalloc
, "nedmalloc is pretty much EOL". In other words, it is unmaintained since 2014.Nevertheless, we rely on
nedmalloc
in Git for Windows because the MINGW toolchain is stuck with an MSVC runtime that comes with amalloc()
implementation that is not quite up to par.I tested with
git repack -q -d -l -A --unpack-unreachable=2.weeks.ago
in a partial clone of the Linux source code, which seems to exercise themalloc()
function a lot. Here is the table for thegcc
builds:#1
#2
#3
#4
#5
Here, MIMALLOC2 is run with
MIMALLOC_RESET_DELAY=1
,MIMALLOC_EAGER_COMMIT=0
andMIMALLOC_RESET_DECOMMITS=1
, which were recommended in mimalloc's issue tracker as potentially speeding up certain workloads.The
mimalloc
column is the fastest on average, and in three of the five rows. Run#2
seems to have been an outlier, and in row#4
the non-default allocator times are all close by. Which suggests thatmimalloc
is a clear winner there.For the MSVC builds, the outcome is a lot more unclear:
#1
#2
#3
#4
#5
First of all, the numbers here all seem lower, but this is a separate run on separate build agents at a different time, under a different load, so it is safer to not compare the
gcc
vs MSVC numbers here (that'll be a fun project on its own, at some stage). Point in case: these numbers are smaller than with a similar benchmark I've run before where no unreachable objects were unpacked. This makes no sense and is probably caused by the variance in the load of different build agents (which is why I run these experiments as much on the same build agent as possible, to have somewhat of an apples-to-apples comparison).The next thing to note is that the default allocator is pretty much the clear winner here, and
mimalloc
fares the worst with default parameters, closely followed bynedmalloc
and bymimalloc2
.The strangest thing about these conclusions is that contrary to my findings almost 3 years ago, the default allocator does not perform terrible in these benchmarks. Obviously I do not have the very same
linux
repository I used for testing anymore (neither the machine on which I ran those tests, nor that machine's successor). But the findings were very clear back then, something along the lines of default allocator runs performing 1.5x - 2x slower thannedmalloc
, and evenmimalloc
slightly but noticeably slower thannedmalloc
.So I tried to repeat the benchmark based on the same Git revision as I had done almost 3 years ago, 144cb18. Skipping the MSVC builds altogether (because I could not quickly make them compile without having to investigate some link issues), here are the
gcc
numbers:#1
#2
#3
In contrast to my original test 3 years ago (which was based on
mimalloc
v1.1.0, see 498594b), I usedmimalloc
v2.0.6 (i.e. the latest available version), which is probably the reason why it performs better now than my recollection would have suggested. However, this benchmark finally seems to recapitulate to some extent what I had seen back then, i.e. thatnedmalloc
performed so much better than the default system allocator (I interpret run#1
as an outlier, kind of "warming up the caches").We will have to be a bit careful comparing these numbers when looking at different workflow runs (or even different matrix jobs), and not draw too strong conclusions, because those are numbers obtained by running on build agents, whose load is naturally very variable and therefore not really suitable for performance benchmarking.
Having said that, I feel confident to draw the following conclusions:
repack
performance a lot more independent from the performance characteristics of themalloc()
implementation that is used.mimalloc
is a suitable replacement fornedmalloc
for thegcc
builds, possibly even slightly improving the performance (but the numbers are too inconclusive to say for certain).nedmalloc
being woefully unsupported (and Git not even using the latest available version because of a similar performance hit as withmimalloc
v1.1.0 when I had originally measured it), and due tomimalloc
having matured even further in the past 3 years, we should switch Git for Windows over fromnedmalloc
tomimalloc
.