Review of public Ref(external count, Thread.Yield, error return)

[dzmitry-lahoda]

Did some view onto, hope you find some useful.

ImTools/src/ImTools/ImTools.cs

Line 534 in 56b7f53

throw new InvalidOperationException(_errorRetryCountExceeded);

Also it is public, it may deliver some caveats to possible users:

1. Spin probably should yield for better CPU https://referencesource.microsoft.com/#mscorlib/system/threading/thread.cs,dd960cd58d3d20c1,references
2. Default counter should in public API, not private hidden surprise in production.
3. Consider return error(value tuple, option, or by ref return) result from Swap instead of throwing exception. Retry exceed could be normal condition on low level.
4. Reproduce in test retry count reached

   ImTools/src/ImTools.UnitTests/RefTests.cs

   Line 7 in 56b7f53

   [TestFixture]

   via one fast and one slow getNewValue. Until getNewValue is hanged and if Thread.Yield used I guess retry could be made infinite by default.
5. Consider make it more private-internal if possible.
6. T is class. Consider adding know primiteves swap like Interlocked for long int etc.

   ImTools/src/ImTools/ImTools.cs

   Line 524 in 56b7f53

   public static T Swap<T>(ref T value, Func<T, T> getNewValue) where T : class

7. ImTools/src/ImTools/ImTools.cs

   Line 489 in 56b7f53

   /// <summary>Compares current Referred value with <paramref name="currentValue"/> and if equal replaces current with <paramref name="newValue"/></summary>

   consider document reference equals instead of equals (to be crystal clear).
8. CompareAndSwap or other like name may be better name

   ImTools/src/ImTools/ImTools.cs

   Line 497 in 56b7f53

   public bool TrySwapIfStillCurrent(T currentValue, T newValue) =>

9. ImTools/src/ImTools/ImTools.cs

   Line 498 in 56b7f53

   Interlocked.CompareExchange(ref _value, newValue, currentValue) == currentValue;

   does compares by reference, but returns by bool depending on possible == https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/equality-comparison-operator. Same seems here

   ImTools/src/ImTools/ImTools.cs

   Line 531 in 56b7f53

   if (Interlocked.CompareExchange(ref value, newValue, oldValue) == oldValue)

10. Consider document usage scenarios, i.e. it seems not for long running operations, not ordered operations (while does not orders getNewValue delegates).
11. Inspire https://www.cl.cam.ac.uk/techreports/UCAM-CL-TR-579.pdf :) and think of laptop Intel with 12 cores or Ryzen with 32 cores https://en.wikipedia.org/wiki/Ryzen (these are commodities now)

[dzmitry-lahoda]

Found similar code in .NET source

        public static X TryEnterWriteLock(ref X value, TimeSpan timeout)
        {
         // https://github.com/dotnet/corefx/blob/3b24c535852d19274362ad3dbc75e932b7d41766/src/Common/src/CoreLib/System/Threading/ReaderWriterLockSlim.cs#L233 

   var tracker = new TimeoutTracker(timeout);

             if (tracker.IsExpired)
                {
                    throw new TimeoutException("Some other lock is hold for too long time");
                }

[dzmitry-lahoda]

I guess throw via Thrower may save some performance of spin. Could check that. Example is jtmueller/Collections.Pooled#8

[dzmitry-lahoda]

These people are not afraid of timeout https://github.com/scalaz/scalaz-zio/blob/master/core/shared/src/main/scala/scalaz/zio/Ref.scala#L42. And use function. May be this Ref should to.

[dzmitry-lahoda]

Another Ref https://github.com/atifaziz/Interlocker/issues

[dadhi]

Need to check it at least, before closing it for good.