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

add stream callback #373

Merged
merged 3 commits into from
Aug 31, 2017

Conversation

BenjaminW3
Copy link
Member

@BenjaminW3 BenjaminW3 commented Aug 11, 2017

possible solution for #368

@psychocoderHPC
Copy link
Member

note: I am two weeks not available and will check all PRs after my holidays.

@BenjaminW3 BenjaminW3 force-pushed the topic-stream-callback branch 2 times, most recently from 685e468 to 22a84a9 Compare August 11, 2017 12:09
@BenjaminW3 BenjaminW3 modified the milestones: Version 0.3.0, Future Aug 11, 2017
@BenjaminW3 BenjaminW3 force-pushed the topic-stream-callback branch 3 times, most recently from 08a7a3b to c9006ab Compare August 11, 2017 15:41
@psychocoderHPC
Copy link
Member

psychocoderHPC commented Aug 28, 2017

Could you please add a short description which concepts are new in this pull request and maybe a code snipped (or pointing to a code snipped) how to use the call backs.

How does the call back works (in which context is the callback executed). Is there a new thread opened and executes the call back code?

@BenjaminW3
Copy link
Member Author

BenjaminW3 commented Aug 28, 2017

There are no new concepts. The alpaka::stream::traits::Enqueue trait and its accessor method alpaka::stream::enqueue are used as is and specialized for callbacks as parameter.
Callback usage looks like this:

alpaka::stream::enqueue(
    stream,
    [&](){
        do_what_you_want_in_here();
    }
);

A usage example can also be found in the new unit test.

Callbacks are always executed within an independent thread. The CPU streams always supported this (unintentionally ;-) ) because it uses a thread pool to execute the tasks. Only for CUDA streams there is new code. CUDA itself calls the callback from within its own thread which does not allow to call CUDA runtime API functions from within the callback. This limitation is lifted by starting a new thread per callback for async CUDA streams and using the waiting thread itself for sync CUDA streams.

@BenjaminW3
Copy link
Member Author

ready for review ;-)

Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

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

I have a performance question

pCallbackSynchronizationData.get(),
0u));

std::thread t(
Copy link
Member

Choose a reason for hiding this comment

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

To create a thread per callback can be very expensive in time. e.g. PIConGPU is spawning over 2000 kernel/memcpy per second and will have over 100 tasks waiting in streams. This means we need to spawn 2k threads/s and have over 100 active threads.

Is it possible to use on thread for all callbacks (waiting in the background), add callbacks to a list and than execute the callback always from the callback thread?

We can move this also to later pull request if it is currently not easy to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, I would like to merge this as is. I would create a follow-up issue for optimizing this.
The most flexible solution would be a thread pool with a queue of ready callbacks. If there is only one thread, the latency would be highest but it would equal your single thread solution, if there are multiple threads, the latency/resource tradeoff can be adapted per use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we merge this and create the follow up ticket? My upcoming event unit tests are based on some of those stream test helpers.

Copy link
Member

Choose a reason for hiding this comment

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

sry I was busy, yes I will merge it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants