dadhi / ImTools

Fast and memory-efficient immutable collections and helper data structures
MIT License
232 stars 10 forks source link

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

Open dzmitry-lahoda opened 6 years ago

dzmitry-lahoda commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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