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.

Sign up for GitHub

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

#265 Update MemoryMappedPageManager memory mapped initiation #388

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#265 Update MemoryMappedPageManager memory mapped initiation #388

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/Paprika/Store/PageManagers/MemoryMappedPageManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,9 @@ public unsafe MemoryMappedPageManager(long size, byte historyDepth, string dir,
_file = new FileStream(Path, FileMode.Open, FileAccess.ReadWrite, FileShare.None, 4096,
PaprikaFileOptions);
}
_mapped = MemoryMappedFile.CreateFromFile(_file, null, 0, MemoryMappedFileAccess.ReadWrite, HandleInheritability.None, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, the mapping will get size from the _file, but if the file is just created, it won't work as expected as one will need to set the length. I don't understand how this should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if we remove this line -

_file.setLength(size);

while creating a new file. We can set it to some safe initial size.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how to answer. We may do all the things as long as the file is properly mapped and written and flushed 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you recommend -

  1. initialize the _file with an initial size of say, 4 MB.
  2. modifying the write and flush operations in a way it first checks the size of the _file and only if it's large enough to store incoming data, proceed as it is. Otherwise, can double the file size until it can accommodate incoming data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you recommend -

  1. initialize the _file with an initial size of say, 4 MB.
  2. modifying the write and flush operations in a way it first checks the size of the _file and only if it's large enough to store incoming data, proceed as it is. Otherwise, can double the file size until it can accommodate incoming data.

@Scooletz What do you think about this approach?


_mapped = MemoryMappedFile.CreateFromFile(_file, null, (long)size, MemoryMappedFileAccess.ReadWrite,
HandleInheritability.None, true);

_whole = _mapped.CreateViewAccessor();
_whole = _mapped.CreateViewAccessor(0, historyDepth * Page.PageSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

The _whole is used as both, the view to ForceFlush when a synchronization happens and as an accessor that provides the pointer _ptr for the whole file. This is why it was set to cover whole file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we introduce a new variable called _viewAccessor for accessing the memory mapped portion as a pointer. And use _whole as it is for ForceFlush?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following. ForceFlush semantics should be that all the pages that were allocated are flushed down and then FSYNC is called. I believe we could create a one time use accessor and flush it. This might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean, inside the ForceFlush method, we can get another accessor for _file that covers the whole file and flush it?

_whole.SafeMemoryMappedViewHandle.AcquirePointer(ref _ptr);
_options = options;

Expand Down