Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Removed Algebra.Release #1417

Conversation

Projects
None yet
3 participants
@mpilquist
Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

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

@mpilquist

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

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

@SystemFw

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

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

@mpilquist

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Contributor

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 functional-streams-for-scala:series/1.0 Feb 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpilquist mpilquist added this to the 1.0.4 milestone Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.