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

Safer disposal #944

Closed
wants to merge 1 commit into from
Closed

Safer disposal #944

wants to merge 1 commit into from

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Oct 30, 2016

Calling dispose on reservation disposes all associated Memory.

Reservation dispose can only dispose itself and Memory derived from it; not other Reservations or owner (reference count only decrements once).

Calling dispose on owner disposes itself and all associated Memory it does not dispose any Reservations or their Memory (reference count only decrements once).

Lifetimes are independent and disposes can't effect each other e.g. you can't copy the Reservation struct and then over dispose the reference.

Full compliance with Dispose pattern

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times.

@benaadams
Copy link
Member Author

Added AutoBufferPool which demonstrates full pooling of both the Array data and the OwnedMemory

/cc @KrzysztofCwalina @davidfowl

@benaadams
Copy link
Member Author

MemoryTests.AutoDispose changed to test overlapping lifetimes and disposal

MemoryTests.ArrayMemoryLifetime changed to test full overlapping lifetimes, disposal with methods throwing ObjectDisposedExceptions at the right times.


if (index > _outstandingReferences.Length)
{
var refs = new uint[index + 1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this store refs in _outstandingReferences?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_outstandingReferences is just a counted bit mask; there is an issue with Empty OwnedMemory as it would keep growing, but that has disposal issues anyway.

Idea in this pattern is if _outstandingReferences array > 1 in length on disposal it is released and set to empty. If it is length == 1 it is kept and cleared; so in reuse if less than 32 reservations were made on it there would be no allocations.

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Oct 31, 2016

This is interesting. I just worry about two things:

  1. The cost of maintaining references table
  2. Being able to implement this cheaply on existing types, i.e. @jaredpar had the idea of creating an interface that would be used by Memory to call methods on "owned memories". This interface would be implemented by T[] and so you could create Memory from T[] with no additional allocation.

cc: @davidfowl, @jkotas, @vancem

@benaadams
Copy link
Member Author

  1. The cost of maintaining references table

Is mostly just a bit mask so should be low; unless you go above 31 reservations when it will allocate (and every 32 reservations on the same OwnedMemory prior to disposal) - but hopefully that should be uncommon.

This interface would be implemented by T[] and so you could create Memory from T[] with no additional allocation.

A sort of lifetime-less shared OwnedMemory beyond OwnedArray? Will have an explore...

@KrzysztofCwalina
Copy link
Member

It's a bitmask but in an array. Are you not concerned with the allocation?

@benaadams
Copy link
Member Author

Are you not concerned with the allocation?

You know my weakness ;-) Also is an extra indirection...

Could dual field it; change to long + array and only allocate array if you expand passed 64 reservations? Probably wouldn't be worse than just an array, but wouldn't be pleasing to eye...

@KrzysztofCwalina
Copy link
Member

Yeah, I was thinking about using an int first (or even not having the _references field) and then switching to an array. Since slots cannot be reused, sooner of later it would allocate (if many reservations are taken from the same memory).

BTW, this test fails (with index out of range):

            OwnedMemory<byte> owned = new OwnedArray<byte>(1024);
            var baseMemory = owned.Memory;
            for(int i=0; i<100; i++) {
                var r = baseMemory.Reserve();
                r.Dispose();
            }

@benaadams
Copy link
Member Author

BTW, this test fails (with index out of range):

Off by one error + missing assingment, adding test

@davidfowl
Copy link
Member

Closing this @benaadams

@davidfowl davidfowl closed this Feb 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants