add windows native aligned malloc + unit test case#19195
add windows native aligned malloc + unit test case#19195opencv-pushbot merged 1 commit intoopencv:masterfrom diablodale:win32AlignAlloc
Conversation
alalek
left a comment
There was a problem hiding this comment.
Well done! Thank you for contribution 👍
| TEST(Core_Allocation, alignedAllocation) | ||
| { | ||
| // iterate from size=1 to approximate byte size of 8K 32bpp image buffer | ||
| for (int i = 0; i < 200; i++) { |
There was a problem hiding this comment.
BTW, some external container outside of for loop was critical in GLIBC case to catch memory fragmentation problem.
std::vector<std::string> external_container;
for (...)
{
...
external_container.emplace_back("test");
cv::fastFree(buf);
}
(or with swapped emplace_back()/fastFree() lines - both patterns can be used in applications)
There was a problem hiding this comment.
I separately tested for fragmentation. Please see the issue for detailed analysis.
|
This patch should go into 3.4 branch first. Please:
Note: no needs to re-open PR, apply changes "inplace". |
Won't be able to do that. Several issues:
Usually, I will not be submitting PRs for the legacy 3.x branch. All the things I do (code, test, analysis, etc.) will be on the current 4.x branch. I do not have the bandwidth to do all the work with a broken 22 year old standard in a branch declared as bugfix-only. I understand you are a maintainer of this project. And I'm a contributor. We have to have a middle-ground where contributions work for us both. 🤝 |
|
OK, I will backport this separately after merge. |
* implements #19147 * CAUTION: this PR will only functions safely in the 4+ branches that already include PR 19029 * CAUTION: this PR requires thread-safe startup of the alloc.cpp translation unit as implemented in PR 19029
|
You wrote
I discourage and caution you not to do this. Why? It is unsafe and you risk Windows crashes. The native Windows aligned APIs used in this PR are unsafe until the thread-safe startup bugs of the core opencv allocator are fixed. One collection of these bugs are #19004. It is a complex bug to fix correctly if you don't use C++11. You would need to create critical sections and/or mutex to protect in needed areas, and put cascaded try/catch handlers throughout the codepath. And all that has to be done very carefully. It will likely reduce performance. Only after those multiple unsafe (thread and exception unsafe) allocator bugs are fixed using C++98, can this PR's code then be applied. If you don't fix those unsafe bugs, then you risk a hard crash of Windows applications. Windows will not allow a mix of deallocators We have a saying in English, "Let sleeping dogs lie." 🐶 It means: if there isn't a current problem...and a change is challenging...it is wise to not make a change. I believe that applies with opencv 3.x. The existing opencv 3.x codebase on windows does not have the startup bug 19004. This is because that 3.x codebase uses |
Implements #19147 by adding native Win32 memory alignment to the opencv core memory allocator and test case. Test data collected prior to PR suggests allocation might be 2% slower. Core maintainer prefers this code be considered for merge. Refer to issue for much detail on accuracy and performance data.
Pull Request Readiness Checklist
Patch to opencv_extra has the same branch name.