Skip to content

Release all pagemap reservations at the very end of the program or DLL #773

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

Merged
merged 13 commits into from
Jun 30, 2025

Conversation

NeilMonday
Copy link
Contributor

Continuation of: #771. I made a new PR since it was a somewhat different concept from the original.

This seems to be a more correct way to cleanup at the end of the program or DLL.

  1. Reservations are tracked in a static vector
  2. This vector is a custom one that use VirtualAlloc and VirtualFree to avoid recursive calls
  3. When it's time to exit, free the reservations we have created
  4. #pragma init_seg(".CRT$XCB") is used to set this translation unit to be very early in the initialization phase. This means that our cleanup function passed to atexit() gets called at the very end, and our cleanup code can work properly.

@NeilMonday
Copy link
Contributor Author

@mjp41 Sorry to bug you again. What do you think of this change?

Also, I can't see how to re-run the failed job. I wanted to re-run since the equivalent jobs for 2022 and 2025 both seem to pass.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

I think this makes sense. I will review again next week in more detail. Here are a few minor comments.

@NeilMonday
Copy link
Contributor Author

I’m installing VS 2019 to test that 32-bit test failure.

@mjp41
Copy link
Member

mjp41 commented May 27, 2025

I’m installing VS 2019 to test that 32-bit test failure.

That failing test is a little flaky. It has an unpredictable memory usage. However, it does cause interesting contention that discovered a bug. So I am not entirely happy disabling it, but I am guessing it is just out of memory. I'll run it again.

@NeilMonday
Copy link
Contributor Author

I’m installing VS 2019 to test that 32-bit test failure.

That failing test is a little flaky. It has an unpredictable memory usage. However, it does cause interesting contention that discovered a bug. So I am not entirely happy disabling it, but I am guessing it is just out of memory. I'll run it again.

Do you know what criteria it uses to determine pass/fail? I can't see where it actually decides this. I got 100% tests passed on 2/2 runs when running on my local machine. I matched the same commands as closely as possible to what the github job did.

mkdir build
cd build
mkdir vs16-x32
cd vs16-x32
cmake ../.. -G "Visual Studio 16 2019" -A Win32   -DSNMALLOC_CI_BUILD=On -DSNMALLOC_RUST_SUPPORT=On
cmake --build .  -- /m /p:Configuration=Debug
ctest -j 2 --interactive-debug-mode 0 --output-on-failure -C Debug --timeout 400


100% tests passed, 0 tests failed out of 70

Total Test time (real) = 500.36 sec

@mjp41
Copy link
Member

mjp41 commented May 29, 2025

The main criteria for success of that test is not crashing. Unfortunately it can use an unbounded amount of memory. And the usage is dependent on the relative timing of operations. I spent a very long time attempting to make a reliable test that found the bug this test discovered, but failed.

I propose we shrink the number of iterations on 32bit Windows. Then hopefully CI passes.

I think there was a comment you haven't addressed about the sizing relative to os pages. It would be nice for this to be pages sized as the minimum. You could make this different from CMake if you have a reason for it to be different?

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

Small change. I think the min sizes should be a bit more pervasive. What do you think the following suggestions?

@NeilMonday
Copy link
Contributor Author

Small change. I think the min sizes should be a bit more pervasive. What do you think the following suggestions?

Yes, all of this makes more sense. I'll just double check locally.

@NeilMonday
Copy link
Contributor Author

@mjp41 Do you know what is causing these failures? They don't seem relevant to my change.

@mjp41
Copy link
Member

mjp41 commented Jun 30, 2025

@mjp41 Do you know what is causing these failures? They don't seem relevant to my change.

I'm investigating today. It is blocking other PRs too.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the changes.

@mjp41 mjp41 enabled auto-merge (squash) June 30, 2025 13:07
@mjp41 mjp41 disabled auto-merge June 30, 2025 13:08
@mjp41 mjp41 enabled auto-merge (squash) June 30, 2025 13:08
@mjp41 mjp41 merged commit e600bf1 into microsoft:main Jun 30, 2025
87 checks passed
mjp41 added a commit that referenced this pull request Jun 30, 2025
#773)

* Release all pagemap reservations at the very end of the program or DLL


Co-authored-by: Neil Monday <neil.monday@amd.com>
Co-authored-by: Matthew Parkinson <mjp41@users.noreply.github.com>
@mjp41 mjp41 mentioned this pull request Jul 1, 2025
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