Skip to content

core: use ::operator delete for RVec heavy storage on Windows#22460

Open
wacfrr wants to merge 6 commits into
root-project:masterfrom
wacfrr:fix-rvec-win32-delete
Open

core: use ::operator delete for RVec heavy storage on Windows#22460
wacfrr wants to merge 6 commits into
root-project:masterfrom
wacfrr:fix-rvec-win32-delete

Conversation

@wacfrr

@wacfrr wacfrr commented Jun 2, 2026

Copy link
Copy Markdown

This Pull request:

Fixes a cross-module/cross-CRT heap allocation mismatch in RVec on Windows platforms. This issue can lead to undefined behaviors, memory corruption, or silent exits when memory allocated inside RVec is freed across module boundaries (e.g., between a pre-compiled DLL and JIT-generated code).

Changes or fixes:

  • math/vecops/src/RVec.cxx: In SmallVectorBase::grow_pod, replaced malloc/realloc/free with ::operator new(std::nothrow) and ::operator delete, guarded by #ifdef _WIN32.

  • math/vecops/inc/ROOT/RVec.hxx: In SmallVectorTemplateBase::grow and destructor-related deallocations, applied the same guarded replacement to ensure all heap operations are unified across the class layout.

These changes are strictly restricted to Windows via platform macros; Linux and macOS targets remain completely untouched to preserve their high-performance native realloc paths.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #22449

@wacfrr wacfrr requested a review from dpiparo as a code owner June 2, 2026 15:51
@jblomer

jblomer commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

This will need a matching change in the RNTuple treatment of RVecs

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 14h 57m 49s ⏱️
 3 862 tests  3 851 ✅   0 💤 11 ❌
77 198 runs  77 080 ✅ 107 💤 11 ❌

For more details on these failures, see this check.

Results for commit 91e7252.

♻️ This comment has been updated with latest results.

@wacfrr

wacfrr commented Jun 3, 2026

Copy link
Copy Markdown
Author

/format

Hi, thanks for triggering the CI!

I have just run the local test suites for both math and RNTuple components on my Windows environment (ctest -C RelWithDebInfo). Fortunately, all tests passed successfully (154/154 passed for math, and all RNTuple test targets passed as well). It seems the basic routines and standard behaviors are working well with this patch.

Regarding the clang-format failure on the CI, I apologize for the noise. My local formatter automatically touched some surrounding legacy code lines, which caused the style check to fail.

I've left a /format command above to see if the project's formatting bot can automatically clean it up. If it doesn't support automatic fixing, please let me know, and I will manually revert the formatting changes on the untouched legacy code lines.

Let's monitor the rest of the CI matrix results to see if there are any other hidden regressions. Thanks again!

I understand that this is a critical modification involving core behaviors, and I would be more than happy to continue our discussion on how to properly align this with the rest of the ecosystem.

@bellenot

bellenot commented Jun 3, 2026

Copy link
Copy Markdown
Member

@wacfrr it seems to be working for df002_dataModel.C. There are some other test failures I want to check before going further...

@bellenot bellenot self-assigned this Jun 5, 2026
@bellenot

bellenot commented Jun 5, 2026

Copy link
Copy Markdown
Member

This will need a matching change in the RNTuple treatment of RVecs

Can you point where in RNTuple the same change should be applied?

@bellenot bellenot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code indentation + use #ifdef instead of #if

Comment thread math/vecops/inc/ROOT/RVec.hxx Outdated
Comment thread math/vecops/inc/ROOT/RVec.hxx Outdated
Comment thread math/vecops/inc/ROOT/RVec.hxx Outdated
Comment thread math/vecops/inc/ROOT/RVec.hxx Outdated
Comment thread math/vecops/src/RVec.cxx Outdated
@bellenot

bellenot commented Jun 8, 2026

Copy link
Copy Markdown
Member

And BTW, maybe we could use the Windows version everywhere (i.e. removing the #ifdef completely) ? As @jblomer suggested. Who else can we ask ?

wacfrr and others added 5 commits June 8, 2026 21:58
Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>
Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>
Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>
Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>
Co-authored-by: Bertrand Bellenot <bellenot@users.noreply.github.com>
@wacfrr

wacfrr commented Jun 8, 2026

Copy link
Copy Markdown
Author

And BTW, maybe we could use the Windows version everywhere (i.e. removing the #ifdef completely) ? As @jblomer suggested. Who else can we ask ?

Thanks for the feedback!

I completely agree that consolidating this into a unified cross-platform implementation is a great long-term goal for the codebase, and removing the #ifdef entirely would make the maintenance much cleaner.

Just a minor thought from my side: while the 2x + 1 growth policy will eventually amortize the heap allocation costs for larger vectors, the current specific layout implementation might still hold a non-trivial performance edge for scenarios heavily dominated by short-lived, small-scale arrays (where we frequently hit the threshold boundary but destroy them before getting the amortized benefits).

Perhaps a comprehensive cross-platform benchmark could help clarify the overall performance impact. Either way, I am very glad to defer to your judgment and that of @jblomer on the best path forward for the architecture. Thanks again for taking the time to test and improve this PR!

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.

[Windows] Macro df002_dataModel.C crashes on exit or exits silently due to potential RVec/Heap issues across DLL boundaries in Cling JIT

3 participants