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

Handle combine #55

Merged
merged 21 commits into from Mar 2, 2022
Merged

Handle combine #55

merged 21 commits into from Mar 2, 2022

Conversation

timcassell
Copy link
Owner

@timcassell timcassell commented Feb 22, 2022

  • Combined MakeReady and Handle while still allowing the stack to unwind. Potential performance improvements #26
  • Reduced size of Promise(<T>)s when T is a small struct.
  • Corrected async Promise continuation to execute on the same thread as the last awaited object.
  • Refactored unit tests to be more reliable with improperly used objects.
  • Updated Progress to always schedule the next waiter on the same context, even after progress has already completed.
  • Optimized All/Merge/Race/First promises to remove a field to reduce memory consumption, and no longer schedule on the execution scheduler.

Current master:

|         Type | Pending | Recursion |       Mean | Allocated | Survived |
|------------- |-------- |---------- |-----------:|----------:|---------:|
|   AsyncAwait |    True |         1 |   4.242 μs |         - |    248 B |
|   AsyncAwait |    True |        10 |  43.471 μs |         - |  2,480 B |
|   AsyncAwait |    True |       100 | 429.178 μs |         - | 24,800 B |
| ContinueWith |    True |         1 |   3.861 μs |         - |     80 B |
| ContinueWith |    True |        10 |  38.541 μs |         - |    800 B |
| ContinueWith |    True |       100 | 386.543 μs |         - |  8,000 B |

This PR:

|         Type | Pending | Recursion |       Mean | Allocated | Survived |
|------------- |-------- |---------- |-----------:|----------:|---------:|
|   AsyncAwait |    True |         1 |   4.127 us |         - |    224 B |
|   AsyncAwait |    True |        10 |  41.251 us |         - |  2,240 B |
|   AsyncAwait |    True |       100 | 409.602 us |         - | 22,400 B |
| ContinueWith |    True |         1 |   3.516 us |         - |    160 B |
| ContinueWith |    True |        10 |  35.678 us |         - |    880 B |
| ContinueWith |    True |       100 | 351.374 us |         - |  8,080 B |

Combined `MakeReady` and `Handle` for single await promises while still allowing the stack to unwind.
Refactored to remove 1 branch and some virtual `HandleProgressListener` calls.
Refactored `AsyncPromiseRef` to use a `[ThreadStatic]` field instead of a counter and Interlocked (will correctly continue on the same thread as the last awaiter with this implementation).
…sVisibleTo("ProtoPromiseTests", AllInternalsVisible = true)]` for unit tests since finalizers are unreliable.

Fixed progress tests.
Cleaned up duplicate progress code and made some virtual calls direct.
Update `BackgroundSynchronizationContext` to add `WaitForAllThreadsToComplete()`.
Refactored `ThreadHelper` to use `TestHelper._backgroundContext`.
@timcassell timcassell self-assigned this Feb 22, 2022
…necessary (but ends up releasing the PromiseRef after the callback instead of before, which can be fixed later for non-catch and non-continuewith).

Removed `ProtoPromiseTests\Threading\ForOldRuntime` folder from tests csproj.
Updated CircleCI config to use `AllowStackToUnwind` instead of `DeveloperMode`.
…xt, even after progress has already completed.
…k properly after the refactor).

Reverted CircleCI config to test against DeveloperMode instead of AllowStackToUnwind.
…behavior instead of the execution scheduler, and removed `ILinked<IProgressInvokable>` fields.
@timcassell timcassell marked this pull request as ready for review March 1, 2022 02:33
@timcassell timcassell merged commit ebed2f1 into master Mar 2, 2022
@timcassell timcassell deleted the handle-combine branch March 2, 2022 09:04
@timcassell timcassell mentioned this pull request Mar 2, 2022
8 tasks
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

1 participant