Skip to content

Fix InexactError() on windows when trying to mmap large (>2GB) files.…#13682

Merged
tkelman merged 1 commit intomasterfrom
jq/win_mmap_fix
Oct 22, 2015
Merged

Fix InexactError() on windows when trying to mmap large (>2GB) files.…#13682
tkelman merged 1 commit intomasterfrom
jq/win_mmap_fix

Conversation

@quinnj
Copy link
Copy Markdown
Member

@quinnj quinnj commented Oct 20, 2015

… Fixes #13560

@quinnj quinnj mentioned this pull request Oct 20, 2015
Closed
tkelman
tkelman reviewed Oct 20, 2015
View reviewed changes
base/mmap.jl Outdated
Copy link
Copy Markdown
Contributor

@tkelman tkelman Oct 20, 2015

Choose a reason for hiding this comment

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

indentation is a little odd here, make sure it's all spaces instead of tabs?

Copy link
Copy Markdown
Member Author

@quinnj quinnj Oct 20, 2015

Choose a reason for hiding this comment

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

I ran make check-whitespace and it seemed to pass. That would catch it, right?

Copy link
Copy Markdown
Contributor

@tkelman tkelman Oct 20, 2015

Choose a reason for hiding this comment

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

check-whitespace doesn't enforce tabs or look at the initial indent of anything, just trailing whitespace at the end of a line

@tkelman tkelman added the system:windows Affects only Windows label Oct 20, 2015
@quinnj
Copy link
Copy Markdown
Member Author

quinnj commented Oct 20, 2015

Whitespace should be more consistent now.

tkelman
tkelman reviewed Oct 20, 2015
View reviewed changes
base/mmap.jl Outdated
Copy link
Copy Markdown
Contributor

@tkelman tkelman Oct 20, 2015

Choose a reason for hiding this comment

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

I think the second input here should stay Cint, ref https://msdn.microsoft.com/en-us/library/windows/desktop/aa366791(v=vs.85).aspx and https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx

@quinnj
Copy link
Copy Markdown
Member Author

quinnj commented Oct 20, 2015

Ok, everything should be properly typed now.

tkelman
tkelman reviewed Oct 20, 2015
View reviewed changes
base/mmap.jl Outdated
Copy link
Copy Markdown
Contributor

@tkelman tkelman Oct 20, 2015

Choose a reason for hiding this comment

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

3 DWORDs and a SIZE_T? https://msdn.microsoft.com/en-us/library/windows/desktop/aa366761(v=vs.85).aspx https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751(v=vs.85).aspx

Copy link
Copy Markdown
Member Author

@quinnj quinnj Oct 20, 2015

Choose a reason for hiding this comment

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

I was going off of: https://msdn.microsoft.com/en-us/library/aa914405.aspx.

Copy link
Copy Markdown
Contributor

@tkelman tkelman Oct 20, 2015

Choose a reason for hiding this comment

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

careful

Windows Mobile 6.5

apparently the mobile version of the winapi has different types. lovely.

Copy link
Copy Markdown
Member Author

@quinnj quinnj Oct 21, 2015

Choose a reason for hiding this comment

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

Ah, good catch. I'll try to update tonight.

tkelman added a commit that referenced this pull request Oct 22, 2015
@tkelman
Merge pull request #13682 from JuliaLang/jq/win_mmap_fix
e989f6c 
Fix InexactError() on windows when trying to mmap large (>2GB) files.…
@tkelman tkelman merged commit e989f6c into master Oct 22, 2015
@tkelman tkelman deleted the jq/win_mmap_fix branch October 22, 2015 00:00
@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Oct 22, 2015

I wonder if we could pick a package in which to check for the APPVEYOR environment variable, and create a really big file to test this when it's set. AppVeyor lets you set scheduled builds so it could run once a day which is pretty useful for tracking.

@quinnj
Copy link
Copy Markdown
Member Author

quinnj commented Oct 22, 2015

Not a bad idea. So how would that work? We'd create a ExtraBaseTests.jl package that would just house extra tests that would get run once a day on nightlies?

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Oct 22, 2015

Something like that, yeah.
@IainNZ has been accumulating a few perf-related things at https://github.com/IainNZ/BasePerfTests.jl which could also use the same treatment as long as they don't take too long to run.

quinnj added a commit that referenced this pull request Oct 31, 2015
@quinnj @tkelman
Fix InexactError() on windows when trying to mmap large (>2GB) files. F…
86d67f8 
…ixes #13560

(cherry picked from commit 9cf995e)
ref #13682
tkelman pushed a commit that referenced this pull request Feb 18, 2016
@Cody-G
patches #13682 to fix memory mapping of large files on Windows
73a572c
tkelman added a commit that referenced this pull request Feb 18, 2016
@tkelman
Merge pull request #15125 from Cody-G/cjg/fix_mmap_win
fabec7b 
patches #13682 to fix memory mapping of large files on Windows
tkelman pushed a commit that referenced this pull request Mar 7, 2016
@Cody-G @tkelman
patches #13682 to fix memory mapping of large files on Windows
3770c61 
(cherry picked from commit 73a572c)
ref #15125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

system:windows Affects only Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@quinnj @tkelman