Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Review Memory<T> architecture #952

Closed
KrzysztofCwalina opened this issue Nov 2, 2016 · 11 comments
Closed

Review Memory<T> architecture #952

KrzysztofCwalina opened this issue Nov 2, 2016 · 11 comments

Comments

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Nov 2, 2016

Not a detailed design, but we should review general architecture of OwnedMemory, Memory, etc.

Here are the main points to discuss:

  1. Relationship between main types: OwnedMemory, Memory, ROMemory, IMemory, IROMemory, IReferenceCounted
  2. Do we want the abstractions? how much abstraction?
  3. Reservation IDs
  4. Reference counting

After the review, I will write down the spec.

@buybackoff
Copy link

buybackoff commented Nov 5, 2016

Could you please add a disposable ReservedMemory<T> with a field Memory<T> instead of DisposableReservation? Similar pattern to this. Now I have to combine DisposableReservation and Memory<T> in a new struct to pass Memory<T> around. Not sure it is the supposed way, and it would be nice to have a built-in API for that.

    public struct ReservedMemory<T> : IDisposable {
        private DisposableReservation _reservation;

        public ReservedMemory(Memory<T> memory) {
            Memory = memory;
            _reservation = memory.Reserve();
        }

        public Memory<T> Memory { get; }

        public void Dispose() {
            _reservation.Dispose();
        }
    }

@benaadams
Copy link
Member

@buybackoff would #944 meet those needs? In particular ReservedMemory

@buybackoff
Copy link

@benaadams Exactly, thanks! Haven't noticed that PR.

@buybackoff
Copy link

Currently Memory<T> equality is missing and struct's equality is used by default. Probably the default way should be what Span.ReferenceEqual is doing, e.g. only owner, offset and length comparison.

@KrzysztofCwalina
Copy link
Member Author

The reservation token cannot have Memory property because Memory is mutable. We would need to have R/O and R/W reservation tokens.

@benaadams
Copy link
Member

Already in #944 as ReservedReadOnlyMemory

@mjp41
Copy link
Member

mjp41 commented Dec 14, 2016

@KrzysztofCwalina I have a question about the OwnedMemory class.

https://github.com/dotnet/corefxlab/blob/master/src/System.Slices/System/Buffers/OwnedMemory.cs#L113-L116

         protected internal virtual DisposableReservation Reserve(ref ReadOnlyMemory<T> memory) 
         { 
             return new DisposableReservation(this, Id); 
         } 

In this function, what is the purpose of memory? It is not used?

It is passed as a parameter in
https://github.com/dotnet/corefxlab/blob/master/src/System.Slices/System/Buffers/ReadOnlyMemory.cs#L54

           public DisposableReservation Reserve() 
           { 
             return _owner.Reserve(ref this); 
           } 

@KrzysztofCwalina
Copy link
Member Author

The idea was that if you all Reserve on a small slice of large memory buffer, the implementation can copy just this small slice to a new buffer and return it in reserve. See https://github.com/dotnet/corefxlab/blob/master/tests/System.Slices.Tests/MemoryTests.cs#L105

But, I am not convinced we want this feature. @davidfowl was going to use it at some point, but I am not sure he does.

@davidfowl
Copy link
Member

It no longer needs to be part of OwnedMemory<T>, ReadableBuffer already has a different version of it.

mjp41 added a commit to mjp41/corefxlab that referenced this issue Jan 12, 2017
This is no longer required according to the discussion in dotnet#952
mjp41 added a commit to mjp41/corefxlab that referenced this issue Jan 12, 2017
This is no longer required according to the discussion in dotnet#952
mjp41 added a commit to mjp41/corefxlab that referenced this issue Jan 17, 2017
This is no longer required according to the discussion in dotnet#952
KrzysztofCwalina pushed a commit that referenced this issue Jan 19, 2017
* 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 #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.
ahsonkhan pushed a commit to ahsonkhan/corefxlab that referenced this issue Jan 28, 2017
* 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.
@ahsonkhan ahsonkhan added this to the 2.1.0 milestone Apr 6, 2017
@KrzysztofCwalina
Copy link
Member Author

closing as I think we settled on the architecture we want.

@ahsonkhan
Copy link
Member

After the review, I will write down the spec.

Tracked by https://github.com/dotnet/corefx/issues/23863

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants