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

Change to dynamic memory allocation/commit on windows #1647

Closed

Conversation

dipinhora
Copy link
Contributor

/cc @Praetonus

This PR implements suggestion 2 from #1614 (comment) to switch to using dynamic memory allocation/commitment for Windows.


This commit changes how the Windows memory allocation/commitment
is done by switching from committing all memory immediately
to only committing memory dynamically as it gets allocated.

This resolves the Windows errors that PR #1614 and PR #1629 have
been encountering related to ponyint_virt_alloc and The paging file is too small for this operation to complete.

This commit changes how the Windows memory allocation/commitment
is done by switching from committing all memory immediately
to only committing memory dynamically as it gets allocated.

This resolves the Windows errors that PR ponylang#1614 and PR ponylang#1629 have
been encountering related to `ponyint_virt_alloc` and `The paging
file is too small for this operation to complete.`
@Praetonus
Copy link
Member

Thanks!

I'd like to wait until I've resolved my concerns with #1621 before merging this. I think I've found an issue in the implementation of pool_block_pull.

@SeanTAllen SeanTAllen force-pushed the master branch 6 times, most recently from 97b1943 to 21d37aa Compare March 11, 2017 14:51
@Praetonus
Copy link
Member

@dipinhora I've opened #1677 to fix the bug I found. This should also fix the errors resolved by this PR. Should this PR also be merged?

@dipinhora
Copy link
Contributor Author

@Praetonus I think so, but I leave that decision to you and others.

The main benefit of this change is dynamic memory commitment as memory is used as opposed to committing the entire memory block immediately at the time of the ponyint_virt_alloc call. Without this change, every thread would commit the full 128 MB of POOL_MMAP upon startup even if the application only ever used a fraction of it. With this change, the memory would be committed as it was actually used and the behavior would be similar to how the posix mmap based allocation works (as far as I understand).

The following comment from https://msdn.microsoft.com/en-us/library/windows/desktop/aa366803(v=vs.85).aspx explains a little more about it:

As an alternative to dynamic allocation, the process can simply commit the entire region instead of only reserving it. Both methods result in the same physical memory usage because committed pages do not consume any physical storage until they are first accessed. The advantage of dynamic allocation is that it minimizes the total number of committed pages on the system. For very large allocations, pre-committing an entire allocation can cause the system to run out of committable pages, resulting in virtual memory allocation failures.

@Praetonus
Copy link
Member

I'm going to mark the subject for discussion during sync.

My biggest concern here is the kernel mode transition performed when calling VirtualAlloc. Doing that on every allocation seems like it would have some performance penalty. Though I'm far from a Windows expert so I may be wrong here.

@Praetonus
Copy link
Member

I was thinking, maybe we could reduce the amount of memory we allocate in each VirtualAlloc on Windows and still commit the memory all at once. Since we'll have to do additional calls to VirtualAlloc anyway compared to the current state of things, it seems to me that allocating memory from the OS in smaller blocks would result in fewer calls and more importantly, keep all the calls when we first get the memory from the OS. Maybe switching to something like 16MB per VirtualAlloc would be reasonable. That'd be 1GB on a 64 cores machine.

@dipinhora
Copy link
Contributor Author

@Praetonus That makes sense and is likely the better approach.

Let me know whenever you make your final decision and I'll close this PR if you decide to go another way.

@Praetonus
Copy link
Member

We'll try to discuss it at the next sync meeting. We didn't have time this week.

@Praetonus
Copy link
Member

I ran into memory problems again for #1726. I'll open a first PR with the change to the amount of memory allocated from the OS, so that we can discuss your PR and mine during the sync meeting today.

I've also noticed that we end up with a lot of small blocks in the global block list. We probably need to do coalescing, I'll open another PR for that.

@Praetonus
Copy link
Member

We discussed the two PRs during sync today. The general sentiment was that the system calls implied by VirtualAlloc would really hurt performance. We decided to merge #1733 instead.

I'm going to close this PR, thanks for raising the initial concern.

@Praetonus Praetonus closed this Mar 22, 2017
@dipinhora
Copy link
Contributor Author

Sounds good. Thanks.

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.

3 participants