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

Better mmap() emulation #486

Merged
merged 3 commits into from Oct 13, 2015
Merged

Better mmap() emulation #486

merged 3 commits into from Oct 13, 2015

Conversation

dscho
Copy link
Member

@dscho dscho commented Oct 13, 2015

Kevin David provided access to his VM where a simple git fetch would produce this error output:

fatal: mmap failed: No error
fatal: write error: Invalid argument

The reason was that several bits of our mmap() emulation left room for improvement. This Pull Request tries to close the gap.

It is not really helpful when a `git fetch` fails with the message:

	fatal: mmap failed: No error

In the particular instance encountered by a colleague of yours truly,
the Win32 error code was ERROR_COMMITMENT_LIMIT which means that the
page file is not big enough.

Let's make the message

	fatal: mmap failed: File too large

instead, which is only marginally better, but which can be associated
with the appropriate work-around: setting `core.packedGitWindowSize` to
a relatively small value.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Often we are mmap()ing read-only. In those cases, it is wasteful to map in
copy-on-write mode. Even worse: it can cause errors where we run out of
space in the page file.

So let's be extra careful to map files in read-only mode whenever
possible.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
On Windows, we have to emulate the fstat() call to fill out information
that takes extra effort to obtain, such as the file permissions/type.

If all we want is the file size, we can use the much cheaper
GetFileSizeEx() function (available since Windows XP).

Suggested by Philip Kelley.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Oct 13, 2015

Cc @kevin-david @phkelley

hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
PAGE_WRITECOPY, 0, 0, NULL);
hmap = CreateFileMapping(osfhandle, NULL,
prot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@buildhive
Copy link

Git for Windows » git #14 SUCCESS
This pull request looks good
(what's this?)

@dscho
Copy link
Member Author

dscho commented Oct 13, 2015

... aaaand the test suite passed here, too.

dscho added a commit that referenced this pull request Oct 13, 2015
@dscho dscho merged commit 423ff0d into git-for-windows:master Oct 13, 2015
@dscho dscho deleted the mmap-no-error branch October 13, 2015 17:28
dscho added a commit that referenced this pull request Oct 19, 2015
dscho added a commit that referenced this pull request Nov 9, 2015
dscho added a commit that referenced this pull request Dec 11, 2015
dscho added a commit that referenced this pull request Jan 5, 2016
dscho added a commit that referenced this pull request Jan 28, 2016
dscho added a commit that referenced this pull request Feb 6, 2016
dscho added a commit that referenced this pull request Feb 23, 2016
dscho added a commit that referenced this pull request Mar 15, 2016
dscho added a commit that referenced this pull request Mar 18, 2016
dscho added a commit that referenced this pull request Mar 29, 2016
dscho added a commit that referenced this pull request Apr 4, 2016
@csware
Copy link

csware commented Apr 22, 2016

@dscho Is this planned to be passed upstream?

@dscho
Copy link
Member Author

dscho commented Apr 22, 2016

@csware yes, of course ;-) Thanks for the reminder.

@dscho
Copy link
Member Author

dscho commented Apr 22, 2016

dscho added a commit that referenced this pull request Apr 30, 2016
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.

None yet

4 participants