Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Removed Algebra.Release #1417

Merged

Conversation

mpilquist
Copy link
Member

@mpilquist mpilquist commented Feb 14, 2019

Prior to this PR, resources were released in two main ways:

  1. Algebra.Release would release a specific resource
  2. Algebra.CloseScope would close a scope, releasing all resources in that scope (and child scopes)

While working on the topic/tagless2 branch, I noticed that we hardly ever hit case (1) above. Almost all resources get released as a result of (2). I added instrumentation to the interpreter to track those conditions and ran our test suite with these results:

Release Type Pre PR Post PR
Explicit 59465 0
Scope Closure 1492331 1545044

Prior to this PR, over 96% of all release releases were due to scope closure.

In this PR, I removed the Algebra.Release constructor and changed Stream.bracket to no longer explicitly release the acquired resource. This broke merge due to an assumption on when a finalizer would be run, but that was easily fixed by explicit introduction of a scope (see the diff). Otherwise, all tests passed.

My motivations:

  1. Simplify reasoning about resource finalization.
  2. Performance & memory improvement as we avoid binding a Release node on every bracket which is typically not used.

@SystemFw
Copy link
Collaborator

I agree with the underlying idea. Will review more thoroughly once I get home.
The nice thing about the tagless branch is that even in the worst case scenario, e.g. we find out it's not going to work out, it has provided value to the current implementation anyway

@pchlupacek
Copy link
Contributor

@mpilquist thats cool. Exactly what was needed to simplify it. Will review over weekend.

@mpilquist
Copy link
Member Author

BTW, I'll look in to the TCP test failure soon -- some time in next couple of days.

@SystemFw
Copy link
Collaborator

@mpilquist is there any common structure to the cases where you had to introduce scope explicitly?

@mpilquist
Copy link
Member Author

Yeah, I'll try to characterize them. There were a couple of cases -- the one in the implementation of merge and a couple in tcp.SocketSpec. In all cases, the pattern has been s.onFinalize(f) such that progress relied on f running relatively immediately. Introducing a scope after the call to onFinalize indicates we don't want to register f in the current scope of some unknown duration but rather finalize ASAP.

Why only these few cases though? I believe that's b/c these are cases where the s.onFinalize(f) stream isn't pulled from (which would introduce a scope when the pull is converted back to a stream) and instead, directly evaluated with no further processing.

@SystemFw
Copy link
Collaborator

SystemFw commented Feb 16, 2019

stream isn't pulled from (which would introduce a scope when the pull is converted back to a stream)

ah ok, that's what I wasn't sure about actually, thanks :)

@SystemFw SystemFw merged commit b0c72b1 into typelevel:series/1.0 Feb 22, 2019
@mpilquist mpilquist added this to the 1.0.4 milestone Mar 1, 2019
@mpilquist mpilquist deleted the topic/remove-explicit-resource-release branch February 18, 2020 12:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants