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

Assorted fixes from the Git for Windows project #654

Merged
merged 6 commits into from
Dec 20, 2022

Conversation

dscho
Copy link
Member

@dscho dscho commented Dec 2, 2022

As of Git for Windows v2.38.0, it uses mimalloc.

To be able to compile it, to allow it to run on Windows Nano Server and to reduce some start-up cost, a couple of changes were made.

This PR backports those fixes into mimalloc proper.

Note: The commits d389795 and cb24a66 were originally done a bit differently in Git for Windows, for convenience using already-available macros to lazy-load functions. Since mimalloc does not have those macros, they were "unrolled by hand" for the purpose of this contribution.

dscho added 6 commits December 3, 2022 00:24
This mostly deletes trailing spaces.

Signed-off-by: Johannes Schindelin <[email protected]>
The `GetNumaProcessorNode()` symbol is not defined in Nano Server's DLLs
(because that function is long deprecated). This causes problems with
any executable that uses mimalloc when trying to run on Nano Server.

Instead of importing this function statically, try to import it
dynamically, and fall back gracefully if it cannot be loaded.

Signed-off-by: Johannes Schindelin <[email protected]>
This format is not supported by MSVC runtimes targeted by the mingw-64
toolchain.

Signed-off-by: Johannes Schindelin <[email protected]>
Let's load the `GetProcessMemoryInfo()` function dynamically. When
needed. If needed.

This is necessary because the start-up cost spent on loading dynamic
libraries is non-negligible.

Note: In contrast to how `os.c` loads libraries and obtains function
addresses, we cannot call `FreeLibrary(hDll)` here because that would
unload the `bcrypt` library before we want to use it.

Signed-off-by: Johannes Schindelin <[email protected]>
Let's not make `bcrypt.dl` a link-time bound library. Instead, load the
`BCryptGenRandom()` function dynamically. When needed. If needed.

This helps reduce the start-up cost of any mimalloc user because the
time spent on loading dynamic libraries is non-negligible.

Note: In contrast to how `os.c` loads libraries and obtains function
addresses, we cannot call `FreeLibrary(hDll)` here because that would
unload the `bcrypt` library before we want to use it.

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]>
@dscho dscho force-pushed the git-for-windows-assorted-fixes branch from 8363e7d to 089e85b Compare December 3, 2022 00:13
@dscho dscho marked this pull request as ready for review December 3, 2022 00:16
@daanx daanx merged commit 59ea84c into microsoft:dev Dec 20, 2022
@daanx
Copy link
Collaborator

daanx commented Dec 20, 2022

Thanks so much! I merged and changed the code a bit; in particular using WriteFile instead of fputs -- can you check if this works for you?
Best,
-- Daan

@dscho dscho deleted the git-for-windows-assorted-fixes branch December 20, 2022 14:22
dscho added a commit to dscho/git that referenced this pull request Dec 20, 2022
To test the changes contributed via
microsoft/mimalloc#654 and further fixed up by
Daan in the `dev` branch (which corresponds to the v1.x track, and which
was merged into the `dev-slice` branch that corresponds to the v2.x
track that we follow).

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member Author

dscho commented Dec 20, 2022

I merged and changed the code a bit; in particular using WriteFile instead of fputs -- can you check if this works for you?

@daanx Yes, it does (I merged the dev-slice changes and ran the Nano Server test).

Thank you!

@dscho
Copy link
Member Author

dscho commented Dec 21, 2022

And Git's regular test suite also passes.

@daanx do you have any plans for a new mimalloc version?

@daanx
Copy link
Collaborator

daanx commented Dec 22, 2022

And Git's regular test suite also passes.

@daanx do you have any plans for a new mimalloc version?

Great -- thanks for testing. Hope to push a fresh release tomorrow if I get to it. But very soon at least.

dscho added a commit to git-for-windows/git that referenced this pull request Jan 12, 2023
I recently [upstreamed the mimalloc fixes we developed in Git for
Windows](microsoft/mimalloc#654), most notably
the [fix for running in Windows Nano
Server](#4074). 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.
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