From a827878d8782c841479474ce24e2b6c82a993bdf Mon Sep 17 00:00:00 2001 From: Tim Date: Sun, 29 Dec 2024 03:10:44 -0500 Subject: [PATCH 1/3] Some AsyncLazy code cleanup. --- .../Internal/PromiseRetainerInternal.cs | 3 ++- .../Utilities/Internal/AsyncLazyInternal.cs | 19 ++++++++----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/Package/Core/Promises/Internal/PromiseRetainerInternal.cs b/Package/Core/Promises/Internal/PromiseRetainerInternal.cs index ce2baa47..6e83881e 100644 --- a/Package/Core/Promises/Internal/PromiseRetainerInternal.cs +++ b/Package/Core/Promises/Internal/PromiseRetainerInternal.cs @@ -199,8 +199,9 @@ internal Promise WaitAsync(short promiseId) return GetWaitAsync(promiseId); } + // Same as WaitAsync, but skips validation. [MethodImpl(InlineOption)] - internal Promise WaitAsyncSkipValidation() + internal Promise WaitAsyncUnsafe() { #if PROMISE_DEBUG return WaitAsync(Id); diff --git a/Package/Core/Utilities/Internal/AsyncLazyInternal.cs b/Package/Core/Utilities/Internal/AsyncLazyInternal.cs index d2eb9735..3d1a9cb8 100644 --- a/Package/Core/Utilities/Internal/AsyncLazyInternal.cs +++ b/Package/Core/Utilities/Internal/AsyncLazyInternal.cs @@ -83,10 +83,6 @@ internal override Promise GetOrStartPromise(AsyncLazy owner, ProgressToken #endif private sealed class LazyPromiseNoProgress : Internal.PromiseRefBase.LazyPromise { - private AsyncLazy _owner; - // TODO: make LazyPromise implement multi await handling directly, instead of allocating a separate object. - internal PromiseRetainer _promiseRetainer; - [MethodImpl(Internal.InlineOption)] private static LazyPromiseNoProgress GetOrCreate() { @@ -125,7 +121,7 @@ internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsNoPro if (lazyFields.IsStarted) { promiseRetainer = lazyFields._lazyPromise.UnsafeAs()._promiseRetainer; - return promiseRetainer.WaitAsyncSkipValidation(); + return promiseRetainer.WaitAsyncUnsafe(); } lazyPromise = GetOrCreate(owner); @@ -134,7 +130,7 @@ internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsNoPro lazyPromise._promiseRetainer = promiseRetainer = PromiseRetainer.GetOrCreateAndHookup(lazyPromise, lazyPromise.Id); // Exit the lock before invoking the factory. } - var promise = promiseRetainer.WaitAsyncSkipValidation(); + var promise = promiseRetainer.WaitAsyncUnsafe(); lazyPromise.Start(lazyFields._factory); return promise; } @@ -190,9 +186,7 @@ protected override void OnComplete(Promise.State state) #endif private sealed class LazyWithProgressPromise : Internal.PromiseRefBase.LazyPromise { - private AsyncLazy _owner; - internal PromiseRetainer _promiseRetainer; - internal Internal.ProgressMultiHandler _progressHandler; + private Internal.ProgressMultiHandler _progressHandler; [MethodImpl(Internal.InlineOption)] private static LazyWithProgressPromise GetOrCreate() @@ -235,7 +229,7 @@ internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsWithP var castedPromise = lazyFields._lazyPromise.UnsafeAs(); castedPromise._progressHandler.Add(progressToken, castedPromise._progressHandler.Id); promiseRetainer = castedPromise._promiseRetainer; - return promiseRetainer.WaitAsyncSkipValidation(); + return promiseRetainer.WaitAsyncUnsafe(); } lazyPromise = GetOrCreate(owner); @@ -246,7 +240,7 @@ internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsWithP lazyPromise._promiseRetainer = promiseRetainer = PromiseRetainer.GetOrCreateAndHookup(lazyPromise, lazyPromise.Id); // Exit the lock before invoking the factory. } - var promise = promiseRetainer.WaitAsyncSkipValidation(); + var promise = promiseRetainer.WaitAsyncUnsafe(); lazyPromise._progressHandler.Add(progressToken, lazyPromise._progressHandler.Id); lazyPromise.Start(lazyFields._factory, new ProgressToken(lazyPromise._progressHandler, lazyPromise._progressHandler.Id, 0d, 1d)); return promise; @@ -320,6 +314,9 @@ partial class PromiseRefBase #endif internal abstract class LazyPromise : PromiseWaitPromise { + protected AsyncLazy _owner; + protected PromiseRetainer _promiseRetainer; + [MethodImpl(InlineOption)] protected void Start(Func> factory) { From 32682c12927720178c6b3bdff1198633588d6463 Mon Sep 17 00:00:00 2001 From: Tim Date: Sun, 29 Dec 2024 03:11:56 -0500 Subject: [PATCH 2/3] Optimized AsyncLazy promise. Fixed ObjectDisposedException arguments. --- .../Internal/PromiseFieldsInternal.cs | 6 +- .../Internal/PromiseRetainerInternal.cs | 118 ++++++++++++------ .../Utilities/Internal/AsyncLazyInternal.cs | 116 ++++++----------- 3 files changed, 124 insertions(+), 116 deletions(-) diff --git a/Package/Core/Promises/Internal/PromiseFieldsInternal.cs b/Package/Core/Promises/Internal/PromiseFieldsInternal.cs index 324a70ea..84df349c 100644 --- a/Package/Core/Promises/Internal/PromiseFieldsInternal.cs +++ b/Package/Core/Promises/Internal/PromiseFieldsInternal.cs @@ -192,12 +192,16 @@ partial class PromiseMultiAwait : PromiseRef private int _retainCounter; } - partial class PromiseRetainer : PromiseRef + partial class RetainedPromiseBase : PromiseRef { private TempCollectionBuilder _nextBranches; private int _retainCounter; } + partial class PromiseRetainer : RetainedPromiseBase + { + } + partial class PromiseWaitPromise : PromiseSingleAwait { } diff --git a/Package/Core/Promises/Internal/PromiseRetainerInternal.cs b/Package/Core/Promises/Internal/PromiseRetainerInternal.cs index 6e83881e..fc04a068 100644 --- a/Package/Core/Promises/Internal/PromiseRetainerInternal.cs +++ b/Package/Core/Promises/Internal/PromiseRetainerInternal.cs @@ -4,8 +4,6 @@ #undef PROMISE_DEBUG #endif -#pragma warning disable IDE0016 // Use 'throw' expression - using Proto.Promises.Collections; using System; using System.Diagnostics; @@ -20,11 +18,9 @@ internal abstract partial class PromiseRefBase #if !PROTO_PROMISE_DEVELOPER_MODE [DebuggerNonUserCode, StackTraceHidden] #endif - internal sealed partial class PromiseRetainer : PromiseRef + internal abstract partial class RetainedPromiseBase : PromiseRef { - private PromiseRetainer() { } - - ~PromiseRetainer() + ~RetainedPromiseBase() { if (!WasAwaitedOrForgotten) { @@ -40,37 +36,62 @@ private PromiseRetainer() { } } [MethodImpl(InlineOption)] - new private void Reset() + new protected void Reset() { _retainCounter = 2; // 1 for dispose, 1 for completion. base.Reset(); } [MethodImpl(InlineOption)] - private static PromiseRetainer GetOrCreate() + protected void ResetForInternalUse() { - var obj = ObjectPool.TryTakeOrInvalid>(); - return obj == InvalidAwaitSentinel.s_instance - ? new PromiseRetainer() - : obj.UnsafeAs>(); + _retainCounter = 1; // 1 for completion/dispose. + _nextBranches = new TempCollectionBuilder(0); + base.Reset(); } [MethodImpl(InlineOption)] - internal static PromiseRetainer GetOrCreateAndHookup(PromiseRefBase previous, short id) + protected void Hookup(PromiseRefBase previous, short id) + => previous.HookupNewPromise(id, this); + + [MethodImpl(InlineOption)] + protected void ResetAndHookup(PromiseRefBase previous, short id) { - var promise = GetOrCreate(); - promise.Reset(); - previous.HookupNewPromise(id, promise); + Reset(); + Hookup(previous, id); // We create the temp collection after we hook up in case the operation is invalid. - promise._nextBranches = new TempCollectionBuilder(0); - return promise; + _nextBranches = new TempCollectionBuilder(0); } [MethodImpl(InlineOption)] private void Retain() => InterlockedAddWithUnsignedOverflowCheck(ref _retainCounter, 1); - internal override void MaybeDispose() + internal void Dispose(short promiseId) + { + lock (this) + { + if (promiseId != Id | WasAwaitedOrForgotten) + { + throw new ObjectDisposedException(nameof(Promise.Retainer), "The promise retainer was already disposed."); + } + ThrowIfInPool(this); + + WasAwaitedOrForgotten = true; + } + MaybeDispose(); + } + + // Same as Dispose, but skips validation. + [MethodImpl(InlineOption)] + protected void DisposeUnsafe() + { + ThrowIfInPool(this); + WasAwaitedOrForgotten = true; + MaybeDispose(); + } + + internal sealed override void MaybeDispose() { if (InterlockedAddWithUnsignedOverflowCheck(ref _retainCounter, -1) != 0) { @@ -87,23 +108,10 @@ internal override void MaybeDispose() // We handle this directly here because we don't add the PromiseForgetSentinel to this type when it is disposed. MaybeReportUnhandledRejection(State); Dispose(); - ObjectPool.MaybeRepool(this); + MaybeRepool(); } - internal void Dispose(short promiseId) - { - lock (this) - { - if (promiseId != Id | WasAwaitedOrForgotten) - { - throw new ObjectDisposedException("The promise retainer was already disposed.", GetFormattedStacktrace(2)); - } - ThrowIfInPool(this); - - WasAwaitedOrForgotten = true; - } - MaybeDispose(); - } + protected abstract void MaybeRepool(); internal override bool GetIsCompleted(short promiseId) { @@ -172,6 +180,12 @@ internal override void Handle(PromiseRefBase handler, Promise.State state) SetCompletionState(state); handler.MaybeDispose(); + HandleBranches(state); + MaybeDispose(); + } + + protected void HandleBranches(Promise.State state) + { TempCollectionBuilder branches; lock (this) { @@ -181,7 +195,6 @@ internal override void Handle(PromiseRefBase handler, Promise.State state) { branches[i].Handle(this, state); } - MaybeDispose(); } internal Promise WaitAsync(short promiseId) @@ -190,7 +203,7 @@ internal Promise WaitAsync(short promiseId) { if (!GetIsValid(promiseId)) { - throw new ObjectDisposedException("The promise retainer was already disposed.", GetFormattedStacktrace(2)); + throw new ObjectDisposedException(nameof(Promise.Retainer), "The promise retainer was already disposed."); } ThrowIfInPool(this); @@ -203,12 +216,9 @@ internal Promise WaitAsync(short promiseId) [MethodImpl(InlineOption)] internal Promise WaitAsyncUnsafe() { -#if PROMISE_DEBUG - return WaitAsync(Id); -#else + ThrowIfInPool(this); Retain(); return GetWaitAsync(Id); -#endif } [MethodImpl(InlineOption)] @@ -225,6 +235,34 @@ private Promise GetWaitAsync(short promiseId) #endif } } + +#if !PROTO_PROMISE_DEVELOPER_MODE + [DebuggerNonUserCode, StackTraceHidden] +#endif + internal sealed partial class PromiseRetainer : RetainedPromiseBase + { + private PromiseRetainer() { } + + [MethodImpl(InlineOption)] + private static PromiseRetainer GetOrCreate() + { + var obj = ObjectPool.TryTakeOrInvalid>(); + return obj == InvalidAwaitSentinel.s_instance + ? new PromiseRetainer() + : obj.UnsafeAs>(); + } + + [MethodImpl(InlineOption)] + internal static PromiseRetainer GetOrCreateAndHookup(PromiseRefBase previous, short id) + { + var promise = GetOrCreate(); + promise.ResetAndHookup(previous, id); + return promise; + } + + protected override void MaybeRepool() + => ObjectPool.MaybeRepool(this); + } } // class PromiseRefBase } // class Internal } \ No newline at end of file diff --git a/Package/Core/Utilities/Internal/AsyncLazyInternal.cs b/Package/Core/Utilities/Internal/AsyncLazyInternal.cs index 3d1a9cb8..2b06f287 100644 --- a/Package/Core/Utilities/Internal/AsyncLazyInternal.cs +++ b/Package/Core/Utilities/Internal/AsyncLazyInternal.cs @@ -97,20 +97,16 @@ private static LazyPromiseNoProgress GetOrCreate(AsyncLazy owner) { var promise = GetOrCreate(); promise._owner = owner; - promise.Reset(); + promise.ResetForInternalUse(); return promise; } - internal override void MaybeDispose() - { - Dispose(); - Internal.ObjectPool.MaybeRepool(this); - } + protected override void MaybeRepool() + => Internal.ObjectPool.MaybeRepool(this); internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsNoProgress lazyFields) { LazyPromiseNoProgress lazyPromise; - PromiseRetainer promiseRetainer; lock (lazyFields) { if (lazyFields.IsComplete) @@ -120,17 +116,14 @@ internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsNoPro if (lazyFields.IsStarted) { - promiseRetainer = lazyFields._lazyPromise.UnsafeAs()._promiseRetainer; - return promiseRetainer.WaitAsyncUnsafe(); + return lazyFields._lazyPromise.WaitAsyncUnsafe(); } lazyPromise = GetOrCreate(owner); lazyFields._lazyPromise = lazyPromise; - // Same thing as Promise.GetRetainer(), but more direct. - lazyPromise._promiseRetainer = promiseRetainer = PromiseRetainer.GetOrCreateAndHookup(lazyPromise, lazyPromise.Id); // Exit the lock before invoking the factory. } - var promise = promiseRetainer.WaitAsyncUnsafe(); + var promise = lazyPromise.WaitAsyncUnsafe(); lazyPromise.Start(lazyFields._factory); return promise; } @@ -147,20 +140,18 @@ internal override void Handle(Internal.PromiseRefBase handler, Promise.State sta protected override void OnComplete(Promise.State state) { + SetCompletionState(state); var lazyFields = _owner._lazyFields; - PromiseRetainer promiseRetainer; if (state != Promises.Promise.State.Resolved) { lock (lazyFields) { // Reset the state so that the factory will be ran again the next time the Promise is accessed. - promiseRetainer = _promiseRetainer; - _promiseRetainer = null; lazyFields._lazyPromise = null; } - promiseRetainer.Dispose(promiseRetainer.Id); - HandleNextInternal(state); + HandleBranches(state); + DisposeUnsafe(); return; } @@ -171,13 +162,11 @@ protected override void OnComplete(Promise.State state) lock (lazyFields) { - promiseRetainer = _promiseRetainer; - _promiseRetainer = null; lazyFields.UnsafeAs()._factory = null; } - promiseRetainer.Dispose(promiseRetainer.Id); - HandleNextInternal(state); + HandleBranches(state); + DisposeUnsafe(); } } // class LazyPromiseNoProgress @@ -202,20 +191,16 @@ private static LazyWithProgressPromise GetOrCreate(AsyncLazy owner) { var promise = GetOrCreate(); promise._owner = owner; - promise.Reset(); + promise.ResetForInternalUse(); return promise; } - internal override void MaybeDispose() - { - Dispose(); - Internal.ObjectPool.MaybeRepool(this); - } + protected override void MaybeRepool() + => Internal.ObjectPool.MaybeRepool(this); internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsWithProgress lazyFields, ProgressToken progressToken) { LazyWithProgressPromise lazyPromise; - PromiseRetainer promiseRetainer; lock (lazyFields) { if (lazyFields.IsComplete) @@ -228,19 +213,16 @@ internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsWithP { var castedPromise = lazyFields._lazyPromise.UnsafeAs(); castedPromise._progressHandler.Add(progressToken, castedPromise._progressHandler.Id); - promiseRetainer = castedPromise._promiseRetainer; - return promiseRetainer.WaitAsyncUnsafe(); + return castedPromise.WaitAsyncUnsafe(); } lazyPromise = GetOrCreate(owner); lazyFields._lazyPromise = lazyPromise; // Same thing as Progress.NewMultiHandler(), but more direct. lazyPromise._progressHandler = Internal.ProgressMultiHandler.GetOrCreate(); - // Same thing as Promise.GetRetainer(), but more direct. - lazyPromise._promiseRetainer = promiseRetainer = PromiseRetainer.GetOrCreateAndHookup(lazyPromise, lazyPromise.Id); // Exit the lock before invoking the factory. } - var promise = promiseRetainer.WaitAsyncUnsafe(); + var promise = lazyPromise.WaitAsyncUnsafe(); lazyPromise._progressHandler.Add(progressToken, lazyPromise._progressHandler.Id); lazyPromise.Start(lazyFields._factory, new ProgressToken(lazyPromise._progressHandler, lazyPromise._progressHandler.Id, 0d, 1d)); return promise; @@ -262,24 +244,22 @@ internal override void Handle(Internal.PromiseRefBase handler, Promise.State sta protected override void OnComplete(Promise.State state) { + SetCompletionState(state); var lazyFields = _owner._lazyFields; - PromiseRetainer promiseRetainer; Internal.ProgressMultiHandler progressHandler; if (state != Promises.Promise.State.Resolved) { lock (lazyFields) { // Reset the state so that the factory will be ran again the next time GetResultAsync is called. - promiseRetainer = _promiseRetainer; - _promiseRetainer = null; progressHandler = _progressHandler; _progressHandler = null; lazyFields._lazyPromise = null; } progressHandler.Dispose(progressHandler.Id); - promiseRetainer.Dispose(promiseRetainer.Id); - HandleNextInternal(state); + HandleBranches(state); + DisposeUnsafe(); return; } @@ -290,8 +270,6 @@ protected override void OnComplete(Promise.State state) lock (lazyFields) { - promiseRetainer = _promiseRetainer; - _promiseRetainer = null; progressHandler = _progressHandler; _progressHandler = null; lazyFields.UnsafeAs()._factory = null; @@ -299,8 +277,8 @@ protected override void OnComplete(Promise.State state) progressHandler.Report(1d, progressHandler.Id); progressHandler.Dispose(progressHandler.Id); - promiseRetainer.Dispose(promiseRetainer.Id); - HandleNextInternal(state); + HandleBranches(state); + DisposeUnsafe(); } } // class LazyWithProgressPromise } // class AsyncLazy @@ -312,10 +290,9 @@ partial class PromiseRefBase #if !PROTO_PROMISE_DEVELOPER_MODE [DebuggerNonUserCode, StackTraceHidden] #endif - internal abstract class LazyPromise : PromiseWaitPromise + internal abstract class LazyPromise : RetainedPromiseBase { protected AsyncLazy _owner; - protected PromiseRetainer _promiseRetainer; [MethodImpl(InlineOption)] protected void Start(Func> factory) @@ -324,7 +301,16 @@ protected void Start(Func> factory) try { var promise = factory.Invoke(); - WaitFor(promise._ref, promise._result, promise._id, new CompleteHandler(this)); + ValidateReturn(promise); + if (promise._ref == null) + { + _result = promise._result; + OnComplete(Promise.State.Resolved); + } + else + { + Hookup(promise._ref, promise._id); + } } catch (OperationCanceledException) { @@ -345,7 +331,16 @@ protected void Start(Func> factory, ProgressToke try { var promise = factory.Invoke(progressToken); - WaitFor(promise._ref, promise._result, promise._id, new CompleteHandler(this)); + ValidateReturn(promise); + if (promise._ref == null) + { + _result = promise._result; + OnComplete(Promise.State.Resolved); + } + else + { + Hookup(promise._ref, promise._id); + } } catch (OperationCanceledException) { @@ -360,35 +355,6 @@ protected void Start(Func> factory, ProgressToke } protected abstract void OnComplete(Promise.State state); - -#if !PROTO_PROMISE_DEVELOPER_MODE - [DebuggerNonUserCode, StackTraceHidden] -#endif - private readonly struct CompleteHandler : IWaitForCompleteHandler - { - private readonly LazyPromise _owner; - - [MethodImpl(InlineOption)] - internal CompleteHandler(LazyPromise owner) - { - _owner = owner; - } - - [MethodImpl(InlineOption)] - void IWaitForCompleteHandler.HandleHookup(PromiseRefBase handler) - { - var state = handler.State; - _owner._result = handler.GetResult(); - _owner.RejectContainer = handler.RejectContainer; - _owner.SuppressRejection = true; - handler.MaybeDispose(); - _owner.OnComplete(state); - } - - [MethodImpl(InlineOption)] - void IWaitForCompleteHandler.HandleNull() - => _owner.OnComplete(Promise.State.Resolved); - } } } // class PromiseRefBase } // class Internal From 2cd664819b46588de1ac2735145879a30b515711 Mon Sep 17 00:00:00 2001 From: Tim Date: Sun, 29 Dec 2024 17:42:17 -0500 Subject: [PATCH 3/3] Don't pool AsyncLazy resources. --- .../Utilities/Internal/AsyncLazyInternal.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/Package/Core/Utilities/Internal/AsyncLazyInternal.cs b/Package/Core/Utilities/Internal/AsyncLazyInternal.cs index 2b06f287..6d1d5c90 100644 --- a/Package/Core/Utilities/Internal/AsyncLazyInternal.cs +++ b/Package/Core/Utilities/Internal/AsyncLazyInternal.cs @@ -86,10 +86,15 @@ private sealed class LazyPromiseNoProgress : Internal.PromiseRefBase.LazyPromise [MethodImpl(Internal.InlineOption)] private static LazyPromiseNoProgress GetOrCreate() { +#if PROMISE_DEBUG || PROTO_PROMISE_DEVELOPER_MODE + // Take from pool for object tracking purposes. var obj = Internal.ObjectPool.TryTakeOrInvalid(); return obj == InvalidAwaitSentinel.s_instance ? new LazyPromiseNoProgress() : obj.UnsafeAs(); +#else + return new LazyPromiseNoProgress(); +#endif } [MethodImpl(Internal.InlineOption)] @@ -102,7 +107,9 @@ private static LazyPromiseNoProgress GetOrCreate(AsyncLazy owner) } protected override void MaybeRepool() - => Internal.ObjectPool.MaybeRepool(this); + // AsyncLazy resources are GC'd after the result is obtained, so it makes less sense to pool the promise object. + // We discard instead of returning to the object pool. + => Internal.Discard(this); internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsNoProgress lazyFields) { @@ -180,10 +187,15 @@ private sealed class LazyWithProgressPromise : Internal.PromiseRefBase.LazyPromi [MethodImpl(Internal.InlineOption)] private static LazyWithProgressPromise GetOrCreate() { +#if PROMISE_DEBUG || PROTO_PROMISE_DEVELOPER_MODE + // Take from pool for object tracking purposes. var obj = Internal.ObjectPool.TryTakeOrInvalid(); return obj == InvalidAwaitSentinel.s_instance ? new LazyWithProgressPromise() : obj.UnsafeAs(); +#else + return new LazyWithProgressPromise(); +#endif } [MethodImpl(Internal.InlineOption)] @@ -196,7 +208,9 @@ private static LazyWithProgressPromise GetOrCreate(AsyncLazy owner) } protected override void MaybeRepool() - => Internal.ObjectPool.MaybeRepool(this); + // AsyncLazy resources are GC'd after the result is obtained, so it makes less sense to pool the promise object. + // We discard instead of returning to the object pool. + => Internal.Discard(this); internal static Promise GetOrStartPromise(AsyncLazy owner, LazyFieldsWithProgress lazyFields, ProgressToken progressToken) {