-
Notifications
You must be signed in to change notification settings - Fork 116
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
Conversation
@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. |
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 think this makes sense. I will review again next week in more detail. Here are a few minor comments.
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.
|
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? |
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.
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. |
@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. |
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.
LGTM, thanks for making the changes.
#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>
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.