OwnedMemory tidy and performance testing #1116

mjp41 · 2017-01-12T17:47:34Z

Main changes

Made Reference Count value not directly accessible, only whether it is zero or not.
Added a setting for whether reservations should use:
- Interlocked Reference Counting (slow),
- Reference Counter (almost safe), or
- Nothing (unsafe).
Added a simple performance test for reservations that does parameter sweep.

Minor tidying to

Remove unused Reserve method.
Make DisposableReservations be able to return the Span, as in the design doc.

mjp41 · 2017-01-12T17:50:04Z

@KrzysztofCwalina I made the System.Slices.Tests part of the performance harness. Should I create a separate project for System.Slices performance?

@davidfowl I removed direct access to an OwnedMemory's reference count, and instead provide a method for checking if it has no references. This seemed to fit with your use case in the Pipelines.

KrzysztofCwalina · 2017-01-12T22:31:09Z

src/System.IO.Pipelines/BufferSegment.cs

@@ -85,11 +85,11 @@ public BufferSegment(OwnedMemory<byte> buffer, int start, int end)

        public void Dispose()
        {
-            Debug.Assert(_buffer.ReferenceCount >= 1);
+            Debug.Assert(!_buffer.HasZeroReferences);


Could we make it to be HasReferences? HasZeroReferences when prefixed with '!' is hard to parse (double negation).

May be confusion with ContainsReferences and ilk? Perhaps HasOutstandingReferences?

Or better keep it in sync with usage HasReservations or change DisposableReservation to DisposableReference

I think DisposableReference and HasOutstandingReferences makes the most sense. As AddReference exists on the API surface as well.

Would AutoReference be better than DisposableReference?

I named it DisposableReference to highlight to users that they need to dispose it.

AutoReference sounds like it deals with disposal for you; would need something like "Destructible Types" dotnet/roslyn#161 for an auto

KrzysztofCwalina · 2017-01-12T22:33:13Z

src/System.Slices/System/Buffers/Memory.cs

@@ -55,7 +55,7 @@ public Memory<T> Slice(int index, int length)

        public Span<T> Span => _owner.GetSpanInternal(_id).Slice(_index, _length);

-        public DisposableReservation Reserve() => new DisposableReservation(_owner, _id);
+        public DisposableReservation<T> Reserve() => new DisposableReservation<T>(_owner, _id);


I used to have this type as a generic type and it was a problem. Sometimes you want to store many reservations on a collection, and generic reservations mean you need many different types of collections. And more generally, the generic argument becomes viral.

So if DisposableReservation is non-generic then it can't give you access to the Span directly. Could you elaborate a little on the use case, as storing reservations in a collection to me sounds dangerous, particularly with the ReferenceCounter.

The idea was that the pipeline would store tokens for all buffer segments,a nd then dispose them all when it's done with the buffer. But I just realized in such scenario, all tokens will be Disposable.

Let's just make (keep) it generic then and see how it feels to use it.

KrzysztofCwalina · 2017-01-12T22:38:16Z

src/System.Slices/System/Buffers/OwnedMemory.cs

+    public enum Mode {RefCount, LocalRefCount, None};
+
+    public class OwnedMemorySettings {
+        public static Mode s_mode = Mode.RefCount;


If you make DisposableReservation non-generic, you could just add this to DisposableReservation, so there is no need for a new type. I know this type is probably just temporary (for experiments), but we try to minimize number of types in our APIs.

Also, s_mode does not follow our naming guidelines. Name it 'Mode'

KrzysztofCwalina · 2017-01-12T22:40:12Z

src/System.Slices/System/Buffers/OwnedMemory.cs

@@ -4,6 +4,12 @@

 namespace System.Buffers
 {
+    public enum Mode {RefCount, LocalRefCount, None};


I would call these

public enum ReferenceCountingMethod { Interlocked, ReferenceCounter, None }

KrzysztofCwalina · 2017-01-12T22:46:47Z

tests/System.Slices.Tests/MemoryTests.cs

+
+
+        [MemberData(nameof(ReservationPerformanceData))]
+        [Benchmark]


Re your question about the project for the tests:

I think it's ok for these tests to be here or in the Benchmarks project. I would not create a separate test project for these.

KrzysztofCwalina · 2017-01-12T22:48:38Z

src/System.Slices/System/Buffers/OwnedMemory.cs

-            _owner.AddReference(_id);
+            switch(OwnedMemorySettings.s_mode) {
+                case Mode.RefCount:
+                    ((IKnown)_owner).AddReference(_id);


If you keep the reservation generic, you don't need the cast here.

KrzysztofCwalina · 2017-01-13T20:00:44Z

src/System.Slices/System/Buffers/OwnedMemory.cs

        }

+        public Span<T> Span => _owner.Span;


Sorry, I just noticed this: Could you at least for now not remove this but instead rename it to DangerousGetSpan().
Even if we settle on refcounting option being the default, we want to have the ability to get span without refcounting in cases where the caller knows that it is safe, e.g. the caller owns the memory lifetime or it is GCed memory.

For clarity which paths are considered Dangerous? For instance, I think Memory.Span is going to be Dangerous, should that also be renamed?
To me only

using(var res = Memory<T>.Reserve()) { var s = res.Span; ... }

Is the only way to get a Span from a Memory that doesn't have Dangerous in the external API?

Yes, we wanted to do the same rename on Memory

Do not allow direct access to the reference count, but instead query if it is zero or not. This is a precursor to allowing more efficient reference counting schemes.

This is no longer required according to the discussion in dotnet#952

We need DisposableReservations to be able to return Spans, so we can scope the access of the span.

There is no instance state for the ReferenceCounter, so make all its methods static.

To enable testing different strategies for managing reservations, introduce a static variable that determines how reservations should be checked.

Added a simple performance test that allows a simple parameter sweep for number of threads, objects, and amount of work. This is not a very realistic example as it does not do the pipelining that is the key use case currently.

KrzysztofCwalina · 2017-01-19T03:23:25Z

@dotnet-bot test this please

KrzysztofCwalina · 2017-01-19T03:23:43Z

BTW, let's merge it and then do the renames later.

mjp41 · 2017-01-19T08:29:19Z

Sounds good. Thanks for the comments @KrzysztofCwalina and @benaadams.

* Remove access to Reference Count Do not allow direct access to the reference count, but instead query if it is zero or not. This is a precursor to allowing more efficient reference counting schemes. * Removed Reserve with ReadOnlyMemory ref This is no longer required according to the discussion in dotnet#952 * Made DisposableReservation return Spans We need DisposableReservations to be able to return Spans, so we can scope the access of the span. * Split disposal from pooling in Memory test. * Made ReferenceCounter methods static There is no instance state for the ReferenceCounter, so make all its methods static. * Enable switching modes for Reservations To enable testing different strategies for managing reservations, introduce a static variable that determines how reservations should be checked. * Added performance test for Reservations Added a simple performance test that allows a simple parameter sweep for number of threads, objects, and amount of work. This is not a very realistic example as it does not do the pipelining that is the key use case currently.

dnfclas added the cla-already-signed label Jan 12, 2017

KrzysztofCwalina reviewed Jan 12, 2017

View reviewed changes

KrzysztofCwalina reviewed Jan 13, 2017

View reviewed changes

mjp41 added 7 commits January 17, 2017 17:50

Remove access to Reference Count

df9be35

Do not allow direct access to the reference count, but instead query if it is zero or not. This is a precursor to allowing more efficient reference counting schemes.

Removed Reserve with ReadOnlyMemory ref

86d42da

This is no longer required according to the discussion in dotnet#952

Made DisposableReservation return Spans

61cce96

We need DisposableReservations to be able to return Spans, so we can scope the access of the span.

Split disposal from pooling in Memory test.

968a9e2

Made ReferenceCounter methods static

1b12408

There is no instance state for the ReferenceCounter, so make all its methods static.

Enable switching modes for Reservations

4fbae33

To enable testing different strategies for managing reservations, introduce a static variable that determines how reservations should be checked.

Added performance test for Reservations

32d79db

Added a simple performance test that allows a simple parameter sweep for number of threads, objects, and amount of work. This is not a very realistic example as it does not do the pipelining that is the key use case currently.

mjp41 force-pushed the ReservationPerf branch from b9d9fbc to 32d79db Compare January 17, 2017 19:20

KrzysztofCwalina merged commit 26903ca into dotnet:master Jan 19, 2017

mjp41 deleted the ReservationPerf branch January 19, 2017 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OwnedMemory tidy and performance testing #1116

OwnedMemory tidy and performance testing #1116

mjp41 commented Jan 12, 2017

mjp41 commented Jan 12, 2017

KrzysztofCwalina Jan 12, 2017

benaadams Jan 13, 2017

benaadams Jan 13, 2017

mjp41 Jan 13, 2017

KrzysztofCwalina Jan 13, 2017

benaadams Jan 13, 2017

KrzysztofCwalina Jan 12, 2017 •

edited

Loading

mjp41 Jan 13, 2017

KrzysztofCwalina Jan 13, 2017 •

edited

Loading

KrzysztofCwalina Jan 12, 2017

KrzysztofCwalina Jan 12, 2017 •

edited

Loading

KrzysztofCwalina Jan 12, 2017

KrzysztofCwalina Jan 12, 2017

KrzysztofCwalina Jan 13, 2017

mjp41 Jan 16, 2017 •

edited

Loading

KrzysztofCwalina Jan 17, 2017

KrzysztofCwalina commented Jan 19, 2017

KrzysztofCwalina commented Jan 19, 2017

mjp41 commented Jan 19, 2017

OwnedMemory tidy and performance testing #1116

OwnedMemory tidy and performance testing #1116

Conversation

mjp41 commented Jan 12, 2017

mjp41 commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjp41 Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina commented Jan 19, 2017

KrzysztofCwalina commented Jan 19, 2017

mjp41 commented Jan 19, 2017

KrzysztofCwalina Jan 12, 2017 •

edited

Loading

KrzysztofCwalina Jan 13, 2017 •

edited

Loading

KrzysztofCwalina Jan 12, 2017 •

edited

Loading

mjp41 Jan 16, 2017 •

edited

Loading