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

RFC: Add windows support for shared memory and SharedArray. #8797

Closed
wants to merge 1 commit into from

Conversation

twadleigh
Copy link
Contributor

  • Added cross-platform (POSIX and windows) interface for shared memory (in base/shmem.jl).
  • Edited SharedArray to use this new interface.
  • Edited the test scripts and documentation to reflect the new support for windows.
  • Verified test/parallel.jl runs successfully on 32-bit and 64-bit windows and 64-bit linux.

@ivarne ivarne added the parallelism Parallel or distributed computation label Oct 24, 2014
@twadleigh twadleigh changed the title Added windows support for shared memory and SharedArray. RFC: Add windows support for shared memory and SharedArray. Oct 24, 2014
@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2014

This builds and passes tests on both win32 and win64 on my machines, very nice. I'm not sure I like the following though:

  • changing the function names and signatures around
  • deleting systemerror("shm_open() failed for " * shm_seg_name, fd_mem <= 0)
  • getting rid of the shortcut return for if prod(dims) == 0
  • getting rid of the exit code from unlink (is it needed any of the times it's used?)
  • duplicating a long line instead of using a default argument for readonly::Bool=false
  • introducing a wrapper type with the generic-sounding SharedMemory name for something specifically mmap-like
  • having different signatures for mmap_array_shm, one using a name, the other using the wrapper type - the shared code for both platforms is so short, this seems to confuse things more than it helps consolidate

Would it hurt or make any difference to use the wide-char versions FileMappingW?

Maybe the way you have it now is the only practical way of doing this, or others think it's fine, or I'm being needlessly picky/conservative. Not sure.

@twadleigh
Copy link
Contributor Author

  • changing the function names and signatures around

If by this you are referring to, for instance, shm_open becoming open_shm, the reason I did this was to emphasize that this new function is an abstraction above the POSIX call with distinct semantics, and not just a thin wrapper.

None of these "changed" functions are part of the current public interface (they are implementation details of SharedArray), so I can't say I'm particularly concerned about breakage. (Others wiser might contradict me, though.) However, I am proposing a public interface in shmem.jl. As such, I'm very concerned that this interface is optimally Julian. This is one area where I'm hoping for a good amount of input from the community.

  • deleting systemerror("shm_open() failed for " * shm_seg_name, fd_mem <= 0)
  • getting rid of the shortcut return for if prod(dims) == 0

Oversights. I will fix these.

  • getting rid of the exit code from unlink (is it needed any of the times it's used?)

The return code is not used in SharedArray, and neither would it be particularly useful to do so, as the only return from shm_unlink yields 0 (other cases raising an error).

I could see it making sense, though, to remove the raising of the error and return a Bool. That said, it may not be worth fussing too much over the semantics of unlink as it is not cross-platform (Windows has no analog).

  • duplicating a long line instead of using a default argument for readonly::Bool=false

I guess I'm still used to the "old-school" means of defaulting in Julia (sooo 2012!). The last significant coding I did in Julia predates the addition of default arguments. I'll update accordingly.

  • introducing a wrapper type with the generic-sounding SharedMemory name for something specifically mmap-like

I'm not sure I understand exactly what you mean here. Maybe you can elaborate.

Both POSIX and windows support distinct concepts for "get shared memory" and "memory map it". Aiming for the largest common denominator in the low-level interface, I wanted to make sure it reflected this commonality. I looked to the Netscape Portable Runtime (NSPR) Named Shared Memory API (and its implementation) for inspiration. It was my intention that its semantics are roughly duplicated in Julia.

It would have been nice if Windows supported a stream-like abstraction for shared memory, as POSIX does, in which case we wouldn't need a new SharedMemory abstraction (we could just use IOStream). It also would have been nice not to have to duplicate functionality from the windows implementation of mmap.jl just to mmap shared memory, but since the windows version of mmap_array in mmap.jl composes both the "get" and "mmap" into one operation, I couldn't see a way to both use it and preserve the "two-step" semantics of the low-level interface.

I'm certainly open to a more elegant solution.

  • having different signatures for mmap_array_shm, one using a name, the other using the wrapper type - the shared code for both platforms is so short, this seems to confuse things more than it helps consolidate

Yes, there is very little shared code. However, that little bit captures the most common use case: get a chunk of shared memory then mmap all of it. The lower-level interface supports doing it in two steps, in case you want to mmap a smaller chunk of it or perform multiple mmaps on the same chunk. The vast majority of the time, though, you will want to do it in one fell swoop (as SharedArray does).

Do you find the overloading itself confusing? If so, I'm certainly open to a renaming.

Would it hurt or make any difference to use the wide-char versions FileMappingW?

POSIX shm_open expects char* names. I chose the FileMappingA as they are the nearer Windows analog. There is no deeper reason than this, though.

@twadleigh
Copy link
Contributor Author

Having thought about it a bit more, I think there might be a way to eliminate the explicit "two-step" nature of the low-level interface without sacrificing the flexibility I was after. I think it would also mean that there wouldn't need to be a separate shmem.jl and the implementation could happily live in mmap.jl.

@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2014

None of these "changed" functions are part of the current public interface (they are implementation details of SharedArray), so I can't say I'm particularly concerned about breakage. (Others wiser might contradict me, though.)

The fact that Julia doesn't have truly private functions means you never know who might be hooking directly into some unexported functionality for whatever custom reason they had. It seems Julia caters to the type of user who tinkers with internals more often than you'd think :) PkgEvaluator will find these things for registered packages, and API breaks are fine especially a) for unexported functions and b) during the dev period. But if you're changing names and interface it does seem like there should be a good reason. These are still pretty close to the posix interfaces, except that the names are different now and the mode is split into 2 booleans.

I guess I'm still used to the "old-school" means of defaulting in Julia (sooo 2012!). The last significant coding I did in Julia predates the addition of default arguments. I'll update accordingly.

Oh. You've been around a lot longer than I have then. :)

It also would have been nice not to have to duplicate functionality from the windows implementation of mmap.jl just to mmap shared memory, but since the windows version of mmap_array in mmap.jl composes both the "get" and "mmap" into one operation, I couldn't see a way to both use it and preserve the "two-step" semantics of the low-level interface.

Yeah, I noticed that too, having touched that piece of code a little while ago. Maybe a comment noting that some of the implementation is common, if the code you're copying has any bugs then we'll need to remember to check both places.

I'm not sure I understand exactly what you mean here. Maybe you can elaborate.

Mostly a concern about limited namespace and generality. A type called SharedMemory could be used for all sorts of things, I'm not sure the use case here is substantial enough that it's absolutely necessary to introduce a new type.

Do you find the overloading itself confusing? If so, I'm certainly open to a renaming.

Yeah, I did. It feels as though it might be more clear to me if you didn't have separate "inner" versions of mmap_array_shm that use the SharedMemory type, instead just have @unix_only and @windows_only versions of mmap_array_shm that take a name and do the whole operation (or most of it). The two different signatures for mmap_array_shm seemed like gratuitous use of multiple dispatch in a case where it makes things less clear, rather than more.

I chose the FileMappingA as they are the nearer Windows analog. There is no deeper reason than this, though.

If the name is a ::String then it could potentially be unicode, though it looks like the name is usually auto-generated as string("/jl", getpid(), int64(time() * 10^9)) rather than visible to a user.

Having thought about it a bit more, I think there might be a way to eliminate the explicit "two-step" nature of the low-level interface without sacrificing flexibility I was after.

Sounds like we might be going the same direction then.

Good stuff. Apparently when I said "kick the tires" what I actually meant was "take apart the engine."

Do you or anyone else happen to have any interesting benchmark code using SharedArrays that we could look at performance on?

@twadleigh
Copy link
Contributor Author

If the name is a ::String then it could potentially be unicode, though it looks like the name is usually auto-generated as string("/jl", getpid(), int64(time() * 10^9)) rather than visible to a user.

That String could be unicode is a good enough reason for me to switch to the wide-char versions. The auto-generated string is just one use of what I hope to be a more generic interface. The next version will reflect that. (Question: does convert(::Type{Ptr{Uint16}},::String) do the right thing, or do I have to do extra marshaling?)

Do you or anyone else happen to have any interesting benchmark code using SharedArrays that we could look at performance on?

I do not. Getting cross-platform support for SharedArray was a precondition for me to invest in using it. @amitmurthy?

Apparently when I said "kick the tires" what I actually meant was "take apart the engine."

I think we are going to have a significantly better implementation for your input.

@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2014

Question: does convert(::Type{Ptr{Uint16}},::String) do the right thing, or do I have to do extra marshaling?

I think if you convert the string to utf16 before sending to to ccall, then ccall should do the marshalling. There are some examples of using wide-char winapi functions in env.jl and file.jl. Testing is a good way to find out.


A = mmap_array(T, dims, s, zero(FileOffset), grow=false)
catch e
print_shmem_limits(prod(dims)*sizeof(T))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should retain the informational printing of shared memory limits on an error. Will help folks debug errors due to system limits faster.

@amitmurthy
Copy link
Contributor

Sorry for the delay in responding.

Windows support is a nice addition, but the Windows API is not something I am familiar with. Other than my comments on the error handling bit above, LGTM.

@twadleigh
Copy link
Contributor Author

@amitmurthy Thanks for taking a look. I'm currently working on a different branch which attempts to preserve as much of the existing code as possible, including the bits you mention.

Do you have any non-trivial examples of the use of SharedArray that we might use for a basic performance benchmark?

@amitmurthy
Copy link
Contributor

No, I don't have any. Maybe @timholy can help?

@timholy
Copy link
Member

timholy commented Oct 30, 2014

I do, but they're integrated into a massive application that uses Images, CUDArt, and a lot of custom code, and wouldn't be at all trivial to separate out into a benchmark. Better to write one from scratch.

@twadleigh
Copy link
Contributor Author

I'll probably write up a toy Poisson solver example at some point then.

@twadleigh
Copy link
Contributor Author

Superceded by #9044.

@twadleigh twadleigh closed this Nov 17, 2014
@twadleigh twadleigh deleted the shmem branch November 17, 2014 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parallelism Parallel or distributed computation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants