[delegate] Refactor how AsyncResult are created for BeginInvoke #2568
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The commit message is empty, there's nothing more than a title. It lacks explanation of the changes and justifications on why they are being done. Changes must have a reason and the commit should tell those. There's an argument being removed because it's no longer used, but there's more going on |
luhenry
force-pushed
the
asyncresult-refactor
branch
3 times, most recently
from
c45003e
to
73ff4a4
Compare
When doing Delegate.BeginInvoke, we return an AsyncResult, and it has to cover 2 possible cases: - the remoting case - the threadpool case (or non-remote case) The code paths both starts at mono_delegate_begin_invoke. In the threadpool case, we call mono_threadpool_ms_begin_invoke. The previous implementation of mono_async_result_new would take the following 5 parameters: - domain - handle: it would always be NULL - state: used in both cases - data: it would always be NULL - object_data: it would only be non-NULL in the threadpool case As 3 out of these 5 parameters are either not used, or used in only in one of the 2 cases, it's easier to get rid of them. They have been replaces with the following parameters: - domain - delegate: the delegate to asynchronously call - state: the state to pass the above delegate - callback: the callback to call after the delegate As for object_data, it is now set in the threadpool case after having created the AsyncResult object. Finally, as the value returned by AsyncResult.Invoke is never used (and can never be used by the threadpool), we can simply set its return type to void.
luhenry
force-pushed
the
asyncresult-refactor
branch
2 times, most recently
from
50231be
to
c4c0b7e
Compare
This is invoked for every BeginInvoke in the runtime. By switching it to managed, we reduce the numer of LOC in the runtime, leading to less possible very hard to track bugs. We also reduce the number of transitions from managed to native and from native to managed, leading to better performance. Another added benefit is exception handling is better managed.
There is no os_handle field on ManualResetEvent, EventWaitHandle or WaitHandle, so f_os_handle would always be NULL.
We try here to reduce even further the number of transitions from managed to native and from native to managed. Exception propagation is also better done, as all the code is managed.
luhenry
force-pushed
the
asyncresult-refactor
branch
from
c4c0b7e
to
a78132b
Compare
|
@ludovic-henry what's the status of this PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.