-
-
Notifications
You must be signed in to change notification settings - Fork 807
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
Support for Per Thread Default Stream (PTDS) #4322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick pass. I have two questions:
- Why does the CUDA Programming Guide say we need to recompile an app with
--default-stream
, while there exists such a simple solution?! - Why moving to PTDS will bring speedup in some of your tests? Is it because the implicit syncs are removed? If so, wouldn't using non-blocking streams also do the job for you? I am very curious! 😛
Thanks for pinging me btw!
Stream.null = Stream(null=True) | ||
Stream.ptds = Stream(ptds=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some logic (likely in BaseStream
?) that requires Stream.null
to be a singleton. If a user explicitly requests for PTDS, I suppose we should just make Stream.null
default to PTDS, instead of creating another special stream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work too, but I'm not sure if it's the right solution for two reasons:
- The null stream may become ambiguous, are we really using the null stream or the per-thread one?
- It's not anymore possible to enforce an explicit usage of the default stream when the environment variable is on, not sure how useful it is to use the default stream in that case anyway but it's probably useful to have that option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's best for the core devs to decide, as I could be overlooking some design considerations. My thoughts are:
Stream.null
in CuPy means the default stream, and previously it meant the legacy one.- If a user explicitly asks for PTDS by setting the env var, it implicitly suggests that the legacy default stream should be avoided to the maximum possible extent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the legacy stream should be avoided when PTDS is requested, but forcing that the Stream.null
is indeed the PTDS stream will leave no choice for the user to fallback to the legacy one if necessary for some reason, so perhaps it's good to still have that possibility regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pentschev I just noticed in CuPy's test suite it's expected that when users do not specify any stream, this is true:
assert cuda.Stream.null == cuda.get_current_stream()
See https://github.com/cupy/cupy/blob/master/tests/cupy_tests/cuda_tests/test_stream.py. I guess this assumption might have propagated to some user codes as well? At least I just recalled I got one 😅 But we don't plan to use PTDS so it's probably OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's interesting, indeed I didn't see that before. I suppose this could pose a problem, so we should probably try to disambiguate that first. I would still prefer to have a possibility to enforce using Stream.null
, even when PTDS is enabled, but I'm happy to change that if the preferred is not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using Stream.null
when PTDS is enabled allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is, but the semantics needs to be clearly defined (which is what we were discussing above -- should Stream.null mean stream 1 (legacy) or 2 (per-thread) when it's enabled?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed and the majority was in favor of leaving Stream.null
as the legacy one.
Anyway, we can just proceed as it is now and revisit it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for discussing that!
cupy/cuda/stream.pyx
Outdated
if null: | ||
self.ptr = 0 | ||
self.ptr = runtime.cudaStreamDefault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even after the long discussion we've been in (numba/numba#5162), I am still a bit uncertain whether it's better to choose runtime.cudaStreamDefault
or runtime.cudaStreamLegacy
here. I think the latter is the status quo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically speaking, the cudaStreamDefault
/cudaStreamNonBlocking
are flags for cudaStreamCreateWithFlags
, but it happens that cudaStreamDefault
is also 0
, which is consistent with the default behavior for when no streams are passed and is also used by many people, CuPy included. On the other hand cudaStreamLegacy
and cudaStreamPerThread
are stream handles and these are the ones that we should be using when specifying a stream explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so this is exactly I was thinking: Don't we expect self.ptr
to be a stream handle and so it should be runtime.cudaStreamLegacy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. The default was 0
, so I thought I would play it safe and avoid replacing that by cudaStreamLegacy
for now. I believe the behavior should be the same though. Maybe it's a good time to replace and test that out, unless someone has any reasons not to do so that I may be overlooking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted this now to runtime.cudaStreamLegacy
, so far everything looks correct on my verification.
cupy_backends/cuda/libs/cublas.pyx
Outdated
elif stream_module.is_ptds_enabled(): | ||
setStream(handle, runtime.cudaStreamPerThread) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that this extra branch (here and in several other places) is not necessary, though I didn't think too hard. The "current stream" in CuPy can simply refer to PTDS if it's enabled when launching a Python interpreter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you already allow get_current_stream_ptr()
to return PTDS if it's enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting we replace those two lines simply by the following?
setStream(handle, stream_module.get_current_stream_ptr())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it's already done above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't, note how above there's if stream_module.enable_current_stream
. The enable_current_stream
is only set when using an explicit stream. However, we could perhaps remove that conditional too, I'm not sure if that would work though, my understanding is that get_current_stream_ptr
returns self.current_stream
, which is undefined until setting a stream, or am I missing something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I do think we should explicitly initialize it to
True
: note that it's a module attribute left uninitialized:
So when would it be False
? If we're forcing an initialization to True
, it becomes a completely unnecessary attribute, as I do see it being set to False
anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it is completely unnecessary to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it, adjusted the setStream
functions and get_current_stream_ptr
. I'm not very happy with the condition in https://github.com/cupy/cupy/pull/4322/files#diff-c7ca9ae40b6014a65e9d4420604c0e83adb8aadd77011ce04fd2026d74f43a01R59-R60 , it doesn't look nice to compare to <void*>0
, and I'm not sure what is the most appropriate/preferred/cleanest way to do that with Cython.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current_stream
could be changed from void*
to intptr_t
IMHO so that we just compare with 0
. I don't have strong opinion on this, but would like to note this is done in one of the two stream
modules, and best to be unified when they are merged one way (#3584) or another (#4322 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this now in ece5c55, seems that this change doesn't require modifications elsewhere.
btw This was my impression too, but I just begin to wonder if it's possible to add a callback to the stream, and let it throw out something that we can capture in Python? CuPy already wraps the stream callback API: |
Where does it say users need to recompile? In https://docs.nvidia.com/cuda/cuda-runtime-api/stream-sync-behavior.html both alternatives are presented. The advantage of compiling with
They would, but using non-blocking streams can be very challenging for RAPIDS, and Dask more specifically, we don't want users to require doing so on their own, so PTDS may be a very convenient solution, although it's still unclear whether we will indeed see major improvements for the average workflow. Below is a Dask example with 1 stream (i.e., thread) vs 4 streams: |
Thanks for sharing that Peter! It's interesting that sometimes a gap shows up on one of the streams. Do we know why that is? No worries if not. Just curious 🙂 |
The GPU has a limited amount of resources, most likely launching yet another concurrent kernel wouldn't fit due to one or multiple sets of resources (e.g., shared memory, registers, etc.). |
I ended up working on #4338 out of curiousity. Note this works: import cupy as cp
def callback(data):
print('I am called!')
data.append('catch me if you can')
def do_work(stream):
data = []
with stream:
// do time consuming work
a = cp.random.random(100)
a *= 3
stream.launch_host_func(callback, data)
stream.synchronize()
assert data[0] == 'catch me if you can'
assert len(data) == 1
s = cp.cuda.Stream()
do_work(s)
cp.cuda.Device().synchronize()
print('done') I think the idea also applies if you change EDIT: fix the snippet |
@leofang thanks for checking the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @pentschev! LGTM except for a few minor points. I think we are left with
- Discussion on the
Stream.null
singleton - Tests
Certainly, thanks for the ping @leofang ! |
This seems to be either a bug or unsupported use case, I can also reproduce it with C++ code. I filed an internal bug ID 200699044 and will update here when I hear back from developers. In the meantime, I think we'll need to discuss what's the best approach here. Both |
Hi Peter, I did some manual tests and I think the "bug" is limited to when we initialize the communicator for all devices in the same process (via # mpirun -n 2 python test_nccl.py
from mpi4py import MPI
import cupy as cp
comm = MPI.COMM_WORLD
rank = comm.Get_rank()
size = comm.Get_size()
cp.cuda.Device(rank).use()
if rank == 0:
n_id = cp.cuda.nccl.get_unique_id()
else:
n_id = None
n_id = comm.bcast(n_id, root=0)
n_comm = cp.cuda.nccl.NcclCommunicator(size, n_id, rank)
a = cp.arange(10)
n_comm.allReduce(a.data.ptr, a.data.ptr,
10*a.dtype.itemsize, cp.cuda.nccl.NCCL_FLOAT64, cp.cuda.nccl.NCCL_SUM,
2) # use stream 2
print(rank, a) Perhaps we could simply print a big warning when |
(btw I found this as I was wondering if the bug happens only when |
Thanks @leofang for further investigating this.
I would be fine with that, but the part that becomes maybe syntactically wrong then is that we moved the meaning of
Indeed, I observed the same in C++ with |
Yes, let's revert to the null stream. |
Indeed, it seems to be a good idea to revert to the status quo (stream 0 being null). |
I reverted it back to |
pfnCI, test this please |
Jenkins CI test (for commit 3cc00ce, target branch master) succeeded! |
cupy/cuda/stream.pyx
Outdated
# TODO(pentschev): move to streamLegacy. This wasn't possible | ||
# because of a NCCL bug that should be fixed in the version | ||
# following 2.8.3-1. | ||
self.ptr = runtime.streamDefault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: streamDefault
is a flag, not a stream macro
self.ptr = runtime.streamDefault | |
self.ptr = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stop using my own arguments from #4322 (comment) against myself! 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ha ha, forgot about that one! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why you shouldn't write things that can be used against yourself, I trapped myself with that one! 😄
Jenkins, test this please |
Jenkins CI test (for commit 6ae421d, target branch master) succeeded! |
I was informed that the fix for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @pentschev for pushing it through.
Thanks for help with reviews here @leofang and @emcastillo ! 😄 |
This PR adds Per Thread Default Stream (PTDS) support to CuPy, entirely controllable at runtime via an environment variable. The advantage of this approach is that multiple builds aren't necessary, greatly simplifying the shipping procedure of CuPy.
I don't currently know if this is the "right" approach for CuPy, but is certainly useful for us to get some experience with PTDS. Right now I've verified this with some pure Python multithreaded code (see below) and Dask, I was able to get a 2x speedup for some Dask workflows but I didn't do any performance testing with standalone CuPy as this is going to vary greatly w.r.t. application, number of threads, etc. As of now, I also don't know how to write tests to ensure that we aren't using the default stream when setting
CUPY_CUDA_PER_THREAD_DEFAULT_STREAM=1
, something I verified with the help ofnsys
andnvprof
.The following code serves as an example, verifiable with
nsys
/nvprof
, each host thread runs on a different stream whenCUPY_CUDA_PER_THREAD_DEFAULT_STREAM=1
when compared to running only on the default stream (shown as stream7
) when the variable isn't set:Note we have a similar effort being done for PTDS support in RMM rapidsai/rmm#633.