ImTools icon indicating copy to clipboard operation
ImTools copied to clipboard

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

Open dzmitry-lahoda opened this issue 5 years ago • 4 comments

Did some view onto, hope you find some useful.

https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools/ImTools.cs#L534

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 https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools.UnitTests/RefTests.cs#L7 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. https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools/ImTools.cs#L524
  7. https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools/ImTools.cs#L489 consider document reference equals instead of equals (to be crystal clear).
  8. CompareAndSwap or other like name may be better name https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools/ImTools.cs#L497
  9. https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools/ImTools.cs#L498 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 https://github.com/dadhi/ImTools/blob/56b7f53a784c3843e4299658592d7b9340153fbc/src/ImTools/ImTools.cs#L531
  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 avatar Jul 26 '18 12:07 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 avatar Jul 26 '18 17:07 dzmitry-lahoda

I guess throw via Thrower may save some performance of spin. Could check that. Example is https://github.com/jtmueller/Collections.Pooled/issues/8

dzmitry-lahoda avatar Jan 22 '19 07:01 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 avatar Jan 23 '19 09:01 dzmitry-lahoda

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

dzmitry-lahoda avatar Feb 15 '19 16:02 dzmitry-lahoda