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

Added windows support for SharedArray #9044

Merged
merged 2 commits into from
Nov 19, 2014
Merged

Conversation

twadleigh
Copy link
Contributor

I made every effort to implement this with the least amount of change to existing code. This includes trying to maximize reuse of the existing windows version of the mmap_array function. One not-so-pleasing side effect of this is a clunkier looking signature with a Union type and a couple of new if-else blocks in the body.

I verified that test/parallel.jl passes on linux-64, windows-32, and windows-64 on my machine.

@amitmurthy
Copy link
Contributor

Awesome. LGTM. Will merge in a day if there are no concerns raised.

cc: @vtjnash

if create
flprotect = ro ? 0x02 : 0x04
mmaphandle = ccall(:CreateFileMappingW, stdcall, Ptr{Void}, (Cptrdiff_t, Ptr{Void}, Cint, Cint, Cint, Ptr{UInt16}),
hdl, C_NULL, flprotect, szfile>>32, szfile&typemax(UInt32), name)
Copy link
Member

Choose a reason for hiding this comment

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

missing utf16 call? or perhaps this is handled automatically now?

Copy link
Contributor

Choose a reason for hiding this comment

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

line 141

Copy link
Member

Choose a reason for hiding this comment

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

ah. good call

@ViralBShah
Copy link
Member

@amitmurthy Just bumping this - You mentioned someone who needed this capability on windows.

@ViralBShah ViralBShah added the system:windows Affects only Windows label Nov 18, 2014
@ViralBShah ViralBShah added this to the 0.4 milestone Nov 18, 2014
amitmurthy added a commit that referenced this pull request Nov 19, 2014
Added windows support for SharedArray
@amitmurthy amitmurthy merged commit 9c9597d into JuliaLang:master Nov 19, 2014
@twadleigh twadleigh deleted the shmem2 branch November 19, 2014 03:28
@amitmurthy
Copy link
Contributor

This should backport cleanly, however, we should let it bake in this branch for a while before doing so.

Adding a @JuliaBackports just for reference.

@timholy
Copy link
Member

timholy commented Nov 19, 2014

Yay, this is great! Thanks, @twadleigh!

@twadleigh
Copy link
Contributor Author

@timholy, I'm very gratified to have been able to contribute something useful to base.

@tkelman
Copy link
Contributor

tkelman commented Nov 19, 2014

would prefer testing it for a while on master before backporting, 0.3.3 is scheduled for pretty soon

@amitmurthy
Copy link
Contributor

That's what I meant.

What is the protocol for adding a @juliabackports and backport_pending label? I added them just to track this commit.

@tkelman
Copy link
Contributor

tkelman commented Nov 19, 2014

both the notification and the label are useful, but for commits that aren't attached to a PR we can only use the notification.

@tkelman
Copy link
Contributor

tkelman commented Nov 24, 2014

The backport window is open now for 0.3.4-pre. This doesn't cherry-pick cleanly and should be done carefully due to things like AbstractString and UInt capitalization. This should be okay to backport since it seems to be working pretty well on master, but maybe would be best if @twadleigh could prepare a PR against release-0.3 for the backported version due to the conflicts.

twadleigh pushed a commit to twadleigh/julia that referenced this pull request Nov 25, 2014
twadleigh pushed a commit to twadleigh/julia that referenced this pull request Nov 25, 2014
@tkelman
Copy link
Contributor

tkelman commented Nov 28, 2014

thanks @twadleigh for preparing the backport. removing the label from here since it's now covered by #9148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants