-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Mimalloc test #47062
base: master
Are you sure you want to change the base?
Mimalloc test #47062
Conversation
The GCBenchmarks suite doesn't seem to show much of a difference here since most allocations it does don't use the |
## jll artifact | ||
MIMALLOC_JLL_NAME := mimalloc | ||
|
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.
## jll artifact | |
MIMALLOC_JLL_NAME := mimalloc | |
# -*- makefile -*- | |
## jll artifact | |
MIMALLOC_JLL_NAME := mimalloc |
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.
Yeah, this will need a bit of love, if we decide it's worth
stdlib/mimalloc_jll/test/runtests.jl
Outdated
@@ -0,0 +1,9 @@ | |||
# This file is a part of Julia. License is MIT: https://julialang.org/license | |||
|
|||
using Test, Libdl, mimalloc_jll |
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.
Libdl
is unused?
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 copied the libuv one, but I think it's wrong in both of them :)
stdlib/mimalloc_jll/test/runtests.jl
Outdated
@testset "mimalloc_jll" begin | ||
ptr = ccall((:mi_malloc, mimalloc), Ptr{Cvoid}, (Int,), 4) | ||
@test ptr != C_NULL | ||
ccall((:mi_free, mimalloc), Cvoid, (Ptr{Cvoid},)) |
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.
Missing the argument?
I just tested this at @JeffBezanson's suggestion and I'm seeing a 50% improvement for some simple
After:
|
I believe this is worth doing, I need to fix windows though. I also wanted to test mimalloc 1.x since there is some comments that it might be faster. Also play with the options a bit. |
IMO we should get this merged once windows works. 1.x might be worth testing, but 2.x is a clear improvement over what we have now so if you get windows working, I would vote to merge and improve from there. |
Note that you are only testing one sample there, the very fastest one. For allocators that keep state among the different executions of the benchmarked function, that number might not tell the whole story. |
Good point.
After:
|
I'm not sure why it's making such a difference here. I wouldn't expect this to go through malloc. And we aren't overriding malloc for everyone. Though if it goes through a GC counted malloc then it makes sense. |
Also windows seems to be overriding it's malloc for everything which is a bit surprising 🤔 |
|
So windows is still broken for some reason :( |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
I tried this PR out on my use case (reading and processing data from TIFFs and saving that data). With this PR:
Master:
Timing was comparable. |
@BioTurboNick Could you share the code, maybe in private. So that I could study what is going on? |
Not sure how much I can share. Is there anything I could run on my end that would collect useful information for you? |
Basically I wanted to see why it was holding on to the memory, in my experience it seems to be equal or better at releasing memory. What OS are you using? For reference? |
I would suggest merging this behind a feature flag ( |
AWS's flavor of Linux |
@gbaraldi, this feels like it is pretty much good to go? Maybe add a NEWS entry? |
I've just done the rebase to fix a merge conflict. I don't think this needs a news as our allocator is an internal detail. |
I think this might need a news option, since it adds the option to switch. And doesn't switch by default. We might want to have more widespread testing and then commit to a switch. |
What's the point of doing this if no one knows about it? |
I'd missed the fact that this was off by default |
Windows has some issues with this PR and I don't know why :( |
The |
Analyzegc just needs me to add that the function pointers aren't safepoints. Windows has a bunch of things in the log. |
Needs rebase? |
What's the status of this? I do see:
It's great that you're supporting mimalloc, and I suppose that it will be bundled with and an official CLI option: --alloc[={default*|mimalloc} but what I had in mind, why can't you replace the allocator with just any e.g.:
I just found this one, and would have liked to test it too: I didn't scan all your changes, a lot seems to have to do with GMP and MPFR, either unrelated, or because they need access to the same implementation of malloc and free as the rest of Julia. I'm a bit confused, why not just use the standard (overridable) libc memory allocation functions? Or at least have that option, to bypass some Julia logic. |
This is just a very rough draft of what changing the malloc for the GC allocations could look like. I used the statically linked version of it because it seemed the easiest.
@kpamnany