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.

Sign up for GitHub

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

Jump to bottom

Enable proper override using MIMalloc #9944

Merged
merged 7 commits into from
Dec 8, 2021
Merged

Enable proper override using MIMalloc #9944

merged 7 commits into from
Dec 8, 2021

Conversation

yuslepukhin
Copy link
Member

Description:
This change enables users to build ORT with mimalloc memory allocator on Windows so the CPUAllocator and all new/deletes are served by mimalloc.

Motivation and Context
mimalloc has shown that gives considerable boost to performance over standard windows allocator.
Note: python API has a problem on shutdown garbage collection in pybind11.

@pranavsharma
Copy link
Contributor

pranavsharma commented Dec 7, 2021 •
edited by yuslepukhin
Loading

Do we've a plan for Linux? I suppose it's going to be a separate PR?

In reply to: 987425400

@yuslepukhin
Copy link
Member Author

Linux is supported via LD_PRELAOD mechanism although can be done statically as well.

In reply to: 987425400

pranavsharma
pranavsharma reviewed Dec 7, 2021
View reviewed changes
tools/ci_build/build.py
@@ -388,8 +388,7 @@ def parse_arguments():
"--use_vstest", action='store_true',
help="Use use_vstest for running unitests.")
parser.add_argument(
"--use_mimalloc", default=['none'],
choices=['none', 'stl', 'arena', 'all'], help="Use mimalloc.")
"--use_mimalloc", action='store_true', help="Use mimalloc allocator")
Copy link
Contributor

@pranavsharma pranavsharma Dec 7, 2021 •
edited by yuslepukhin
Loading

Choose a reason for hiding this comment

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

Are there any internal teams that rely on any of these specific choices? May be the ones who introduced them in the first place? Might want to give them a headsup? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually checked. Nobody is using it currently.

Copy link
Member

Choose a reason for hiding this comment

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

What if someone is building from source ?

Copy link
Member Author

Choose a reason for hiding this comment

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

mimalloc is only available for building from source.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so what if someone (3P) is building from source after each release ? They might find to their surprise that this option that existed previously doesn't exist anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no way to know about 3P usage. We can document this in the release notes since this might indeed break existing (if any) 3P workflows.

@pranavsharma
Copy link
Contributor

pranavsharma commented Dec 7, 2021 •
edited by yuslepukhin
Loading

Linux is supported via LD_PRELAOD mechanism although can be done statically as well.

In reply to: 987425400

Ok, let's document how mimalloc can be used with ORT for both Windows and non-Windows envs.

In reply to: 987450755

RyanUnderhill
RyanUnderhill reviewed Dec 7, 2021
View reviewed changes
cmake/onnxruntime_common.cmake
@@ -75,6 +75,14 @@ file(GLOB onnxruntime_common_src CONFIGURE_DEPENDS
${onnxruntime_common_src_patterns}
)

# Remove new/delete intercept. To deal with memory leaks
Copy link
Member

@RyanUnderhill RyanUnderhill Dec 7, 2021 •
edited by yuslepukhin
Loading

Choose a reason for hiding this comment

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

Will memory leak detection be off by default? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed off line

onnxruntime/core/framework/allocator.cc
#else
int ret = mi_posix_memalign(&p, alignment, size);
if (ret != 0)
ORT_THROW_EX(std::bad_alloc);
Copy link
Member

@RyanUnderhill RyanUnderhill Dec 7, 2021 •
edited by yuslepukhin
Loading

Choose a reason for hiding this comment

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

Do the other #if cases also throw std::bad_alloc? #Resolved

hariharans29
hariharans29 reviewed Dec 7, 2021
View reviewed changes
cmake/external/mimalloc.cmake
@@ -10,6 +10,7 @@ include_directories(${mimalloc_root_dir}/include)

option(MI_OVERRIDE "" OFF)
option(MI_BUILD_TESTS "" OFF)
Copy link
Member

@hariharans29 hariharans29 Dec 7, 2021 •
edited by yuslepukhin
Loading

Choose a reason for hiding this comment

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

Are lines 3-8 needed now ? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

@hariharans29
Copy link
Member

hariharans29 commented Dec 7, 2021 •
edited by yuslepukhin
Loading

"all new/deletes are served by mimalloc" - So the new/delete in the default allocator used by the STL containers will also be overridden right ?

In reply to: 987552314

In reply to: 987552314

@yuslepukhin
Copy link
Member Author

Actually, the original override did not target Linux. So there are #ifdefs in the cmake. I have also not tested that.

In reply to: 987450755

@yuslepukhin
Copy link
Member Author

Yes, all new/deletes are overriden.

In reply to: 987552314

@hariharans29
Copy link
Member

hariharans29 commented Dec 7, 2021 •
edited by yuslepukhin
Loading

Yes, all new/deletes are overriden.

In reply to: 987552314

Just curious: In the target model that this change is primarily intended for, are there any STL containers that are used a lot in Run() ? If there are, if we sub in the mi_stl_allocator instead of the default allocator they use (all of this gated by this build flag of course), does that give additional gains ?

In reply to: 988208851

@pranavsharma
Copy link
Contributor

LGTM 👍

snnn
snnn reviewed Dec 8, 2021
View reviewed changes
cmake/onnxruntime_framework.cmake
@@ -63,6 +63,11 @@ if (onnxruntime_ENABLE_TRAINING)
target_include_directories(onnxruntime_framework PRIVATE ${DLPACK_INCLUDE_DIR})
endif()
onnxruntime_add_include_to_target(onnxruntime_framework onnxruntime_common onnx onnx_proto ${PROTOBUF_LIB} flatbuffers)

if (onnxruntime_USE_MIMALLOC)
target_link_libraries(onnxruntime_framework mimalloc-static)
Copy link
Member

Choose a reason for hiding this comment

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

First, after "onnxruntime_framework", you should add "PRIVATE" or "PUBLIC" keyword.

target_link_libraries(onnxruntime_framework PRIVATE mimalloc-static)

However, I suggest you add it to https://github.com/microsoft/onnxruntime/blob/master/cmake/CMakeLists.txt#L938 instead. To make sure they are in order. https://github.com/microsoft/onnxruntime/blob/master/docs/cmake_guideline.md#dont-call-target_link_libraries-on-static-libraries

@yuslepukhin yuslepukhin merged commit a7f649d into master Dec 8, 2021
@yuslepukhin yuslepukhin deleted the yuslepukhin/mimalloc branch December 8, 2021 01:57
yuslepukhin added a commit that referenced this pull request Dec 8, 2021
@yuslepukhin
Document on the usage of mimalloc allocator
25af000 
[Enable proper mimalloc override](#9944) PR has fixed a number of issues with regards to building onnxruntime with mimalloc. This PR documents usage of mimalloc.
@yuslepukhin yuslepukhin mentioned this pull request Dec 8, 2021
Merged
natke pushed a commit that referenced this pull request Dec 8, 2021
@yuslepukhin
Document on the usage of mimalloc allocator (#9974)
50127fe 
[Enable proper mimalloc override](#9944) PR has fixed a number of issues with regards to building onnxruntime with mimalloc. This PR documents usage of mimalloc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanUnderhill RyanUnderhill RyanUnderhill left review comments

@hariharans29 hariharans29 hariharans29 left review comments

@pranavsharma pranavsharma pranavsharma left review comments

@snnn snnn snnn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yuslepukhin @pranavsharma @hariharans29 @snnn @RyanUnderhill