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

Update mimalloc to v2.0.9 #4211

Merged
merged 8 commits into from
Jan 12, 2023
Merged

Conversation

dscho
Copy link
Member

@dscho dscho commented Jan 11, 2023

I recently upstreamed the mimalloc fixes we developed in Git for Windows, most notably the fix for running in Windows Nano Server. In the meantime, that PR was accepted and a new version was created from it (that also includes a couple of other fixes). Let's integrate the latest version.

dscho added 3 commits January 11, 2023 14:49
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]>
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]>
@dscho dscho self-assigned this Jan 11, 2023
Copy link

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the first time we are using mimalloc as our primary allocator?

I read the changes "around" the import carefully, which were sensical adaptations to Git's build system.

dscho added 5 commits January 11, 2023 21:29
This commit imports mimalloc's source code as per v2.0.9, 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]>
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]>
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]>
This topic vendors in mimalloc v2.0.9, a fast allocator that allows Git
for Windows to perform efficiently.

Switch Git for Windows to using mimalloc instead of nedmalloc
@dscho dscho force-pushed the update-mimalloc-to-v2.0.9 branch from f29b19d to 096b848 Compare January 11, 2023 20:30
@dscho
Copy link
Member Author

dscho commented Jan 11, 2023

So this is the first time we are using mimalloc as our primary allocator?

No, we're using it already since v2.38.0.

I read the changes "around" the import carefully, which were sensical adaptations to Git's build system.

👍

I'll merge this PR tomorrow, I think, to have a snapshot with the cURL version that fixes #4194.

@dscho dscho merged commit b3520db into git-for-windows:main Jan 12, 2023
@dscho dscho deleted the update-mimalloc-to-v2.0.9 branch January 12, 2023 08:26
@dscho dscho modified the milestones: Next release, v2.39.1 Jan 12, 2023
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.

2 participants