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.

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

Do not release frozen segment if we failed extending it #94450

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

MichalStrehovsky
Copy link
Member

Maybe I'm missing something, but if we try to commit more of the reserved segment and this fails, it doesn't feel right to release the whole thing. The previously committed part of the segment is still in use.

Maybe I'm missing something, but if we try to commit more of the reserved segment and this fails, it doesn't feel right to release the whole thing. The previously committed part of the segment is still in use.
@EgorBo
Copy link
Member

EgorBo commented Nov 7, 2023

This was a suggestion from @jkotas:

image

I don't have a strong opinion here, presumably if Commit fails we're in a bad state anyway?

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Nov 7, 2023

This was a suggestion from @jkotas:

That comment seems to be for the call in the constructor of FrozenObjectSegment - i.e. we reserved and immedially try to commit, but fail there. Nobody can be using that memory yet so we can just have to release.

But the place where I'm deleting the free is in TryAllocateObject. Unless I'm missing something, we are already using this segment, we just failed to extend the commit range of it. VirtualAlloc (on Windows at least) would fail like that if we're in low memory situation (reserve will succeed even for unreasonable amounts of memory, but actually commiting physical storage can fail).

@EgorBo
Copy link
Member

EgorBo commented Nov 7, 2023

That comment seems to be for the call in the constructor of FrozenObjectSegment

Ah, right, then sounds OK to have it, unless @jkotas has an opposite opinion

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Good catch

@jkotas jkotas merged commit 021ecb8 into main Nov 7, 2023
109 of 111 checks passed
@jkotas jkotas deleted the MichalStrehovsky-patch-1 branch November 7, 2023 18:10
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants