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

Weak reference concurrent pool #10787

Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Oct 25, 2023

The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.

The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
@gregw gregw requested review from sbordet and lorban October 25, 2023 23:16
@gregw
Copy link
Contributor Author

gregw commented Oct 25, 2023

@sbordet @lorban What is wrong with this approach?

Once an entry is acquired, the pool only holds a weak reference to it. Once it is released it creates a hard reference to keep it in GC.

I do not think there is a race, as before... what am I missing?

gregw added 8 commits October 26, 2023 10:20
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
back link may be hard
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
removed ThreadLocal cache
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added Leak test
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
@gregw
Copy link
Contributor Author

gregw commented Oct 26, 2023

@sbordet @lorban OK it was not as simple as it looked at first.... but I think I found the race (could be acquired and freed before it was held after release). I think I have fixed it with a spin test (scary). It now works for multiplexed pools as well. I removed the cache because I think it is an unnecessary complexity.

@gregw gregw requested a review from lachlan-roberts October 26, 2023 05:40
lorban and others added 2 commits October 26, 2023 11:04
…g configurable

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
@gregw
Copy link
Contributor Author

gregw commented Oct 26, 2023

@lorban I reverted to use a weak pool by default (with system property to change default). Whilst we are an experimental branch, we want to test it as much as possible. Perhaps once we merge, we switch the default (and have a better way of setting it).
I'm also using concrete Holder classes rather than if(weak) tests.

@lorban
Copy link
Contributor

lorban commented Oct 27, 2023

@gregw that's cool, and I currently have no strong opinion about whether we should set the pool as weak by default or not.

The reason why I made weak ref configurable is to measure the perf impact of this PR, so I want to compare in order:

  • mainline vs this PR with hard refs only
  • this PR with hard refs only vs this PR with weak refs
  • mainline vs this PR with weak refs
    to make sure this doesn't regress perf in any meaningful way, as the concurrent pool really is perf critical.

I don't mind your modifications; as I can configure an ArrayByteBufferPool with weak or strong references at will the default doesn't impact my perf testing.

@lorban
Copy link
Contributor

lorban commented Oct 27, 2023

Perf report (so far):

This change has no measurable impact, no matter if weak referencing is enabled or not. The JIT inlines the entire buffer acquisition code in a single blob and most of the time is spent, in order: iterating the list, CASing the BiInteger in tryAcquire, updating the ref counter of the RetainableByteBuffer.

Basically, the cost stayed where it was expected to be, so we can concentrate on making sure the feature works. 👍

The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
no race in tests
@gregw
Copy link
Contributor Author

gregw commented Oct 28, 2023

@lorban so I'm doing a risk analysis for this, and I think the fundamental approach is a lot safer than before, as we rely on less moving parts.
I don't think there is any risk if we keep a strong link accidentally, as that is just case we have today and we expect the application to release.
Ditto if we don't keep a strong link when we should, then we might cleanup an entry we did not need to... but only if the application doesn't have a strong link. So it would just be an idle entry shrink... not harm.

The only problem I can see is the spin wait to see a string link before it is cleaned. If we get that wrong, then we spin forever! My concern is what if between one spin and the next, the entry is acquired, released, acquired and released? Do we need a similar spin on the hold method, so that it waits to see the previous free? That will prevent multiple acquire/release cycles from happening during a spin.... actually I think I will implement that now!

@gregw gregw marked this pull request as ready for review October 29, 2023 21:02
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
no race in tests
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

I think this logic is correct, and unless I missed something, the race condition that you fix with a spin loop in hoder can only happen under one cirmunstance: when a release makes the multiplex counter go to 0 at the same time an acquire moves it to 1.

In that case, hold() must execute first so the concurrent free() call stays in the spin loop to wait for that moment before doing its job.

But I see no reason that would make hold() spin as I can't see a race condition that would trigger this need.

Wether I'm right or wrong, these new states and their transitions should be very carefully documented.

{
Pool.Entry<P> getEntry();

void hold();
Copy link
Contributor

Choose a reason for hiding this comment

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

How about renaming this method harden()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer release() as it matches where it is called from.

Copy link
Contributor Author

@gregw gregw Oct 30, 2023

Choose a reason for hiding this comment

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

@sbordet but it is not a release. A release only reduces the multiplexed reference count and only if that goes to zero do we create a non-weak reference. So it is called from release, but is not the same.
Maybe lastRelease or fullyReleased ? I don't mind harden


void hold();

void free();
Copy link
Contributor

Choose a reason for hiding this comment

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

How about renaming this method weaken()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer acquire() as it matches where it is called from.

Copy link
Contributor Author

@gregw gregw Oct 30, 2023

Choose a reason for hiding this comment

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

@sbordet again it is not an acquire. perhaps firstAcquire? I don't mind weaken

{
Pool.Entry<P> getEntry();

void hold();
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer release() as it matches where it is called from.


void hold();

void free();
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer acquire() as it matches where it is called from.

holder = pool.weak ? new WeakHolder<>(this) : new StrongHolder<>(this);
}

private Holder<E> getHolder()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just reference the final field and remove the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

* @param maxDirectMemory the max direct memory in bytes, -1 for unlimited memory or 0 to use default heuristic
* @param weakPool true if the pooled buffers should be weakly referenced upon acquisition, false otherwise
*/
public ArrayByteBufferPool(int minCapacity, int factor, int maxCapacity, int maxBucketSize, long maxHeapMemory, long maxDirectMemory, boolean weakPool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't really like the telescopic additional "weakPool" parameter -- seems an implementation detail that leaks many layers up, and if we change our mind it'll be part of the public APIs for a long time.

Copy link
Contributor Author

@gregw gregw Oct 30, 2023

Choose a reason for hiding this comment

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

@sbordet Isn't this at odds with your previous comment about preferring "explicit configuration". How would you configure this if not by constructor parameter? Should we just have a different classes for Weak pools?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gregw the comments above are now out of date, and the weakPool parameter is never used, so please remove it.

The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
@gregw gregw requested review from sbordet and lorban October 31, 2023 02:48
@lorban
Copy link
Contributor

lorban commented Oct 31, 2023

@gregw @sbordet I think we now reached the point where we all agree that this idea is technically sound, will work, and is worth implementing.

Besides naming (I'll skip that subject), there are still two points of disagreement or uncertainties:

  • What should be done when a leak is detected to help report the problem?
  • How this new feature should be made configurable?

Here are my current positions:

  • When a leak is detected, we shouldn't do more than incrementing a counter exposed via JMX. The kind of information we can get from the ConcurrentPool is IMHO insufficient to track the source of a leak. At least, in the context of the retainable buffers pool where acquire(), retain() and release() calls all need to be tracked to understand where the cause of the leak might be.
  • I'm tempted by the idea that weak referencing shouldn't be made configurable at all, and ConcurrentPool should have this feature always on. My top reason for this preference is that ConcurrentPool already is a complex piece of lock-free code that we should keep as simple as possible.

Thoughts?

@gregw
Copy link
Contributor Author

gregw commented Oct 31, 2023

@gregw @sbordet I think we now reached the point where we all agree that this idea is technically sound, will work, and is worth implementing.

+1

Besides naming (I'll skip that subject), there are still two points of disagreement or uncertainties:

On naming I can live with any of hold/free, strengthen/weaken, lastRelease/firstAcquire but not release/acquire

  • What should be done when a leak is detected to help report the problem?
  • How this new feature should be made configurable?

Here are my current positions:

  • When a leak is detected, we shouldn't do more than incrementing a counter exposed via JMX. The kind of information we can get from the ConcurrentPool is IMHO insufficient to track the source of a leak. At least, in the context of the retainable buffers pool where acquire(), retain() and release() calls all need to be tracked to understand where the cause of the leak might be.

I think that is a reasonable default mechanism.

However, I do think we should at least include something like my DebugWeakHolder, so that extra info can be obtained without the need to change code. However, I'd probably change it to make it a static determination of debug, to give the JIT every opportunity to optimize away virtual calls (does that work?). But for now, I think we should make this debug mechanism private and not extensible, as once protected it is part of the API.

  • I'm tempted by the idea that weak referencing shouldn't be made configurable at all, and ConcurrentPool should have this feature always on. My top reason for this preference is that ConcurrentPool already is a complex piece of lock-free code that we should keep as simple as possible.

+1

So long as we provide alternate classes that do simple or null pooling, then it is best to make our main ConcurrentPool implement the best algorithm we know and to not try to be too configurable unless there is good reason.

gregw added 3 commits November 1, 2023 07:55
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
@lorban lorban added this to the 12.0.x milestone Nov 1, 2023
@lorban lorban linked an issue Nov 1, 2023 that may be closed by this pull request
@lorban
Copy link
Contributor

lorban commented Nov 1, 2023

@gregw I don't think DebugWeakHolder adds much value, if any, so I don't think it's worth the extra complexity in ConcurrentPool. Better leave leak tracking to the layer above, like ArrayByteBufferPool.Tracking IMHO.

gregw and others added 3 commits November 2, 2023 09:39
The Pool is now a list of Holder instances, each with a WeakReference to an Entry and an optional hard reference.
free on acquire
added warnings
* Removed unused boolean parameter "weakPool" in ArrayByteBufferPool and updated javadocs.
* Improved implementation of ConcurrentPool.Entry.tryEnable().
* Added javadocs for ConcurrentPool.Holder.free() and hold().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
* @param maxDirectMemory the max direct memory in bytes, -1 for unlimited memory or 0 to use default heuristic
* @param weakPool true if the pooled buffers should be weakly referenced upon acquisition, false otherwise
*/
public ArrayByteBufferPool(int minCapacity, int factor, int maxCapacity, int maxBucketSize, long maxHeapMemory, long maxDirectMemory, boolean weakPool)
Copy link
Contributor

Choose a reason for hiding this comment

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

@gregw the comments above are now out of date, and the weakPool parameter is never used, so please remove it.

Comment on lines 493 to 494
if (!acquire)
getHolder().hold();
Copy link
Contributor

Choose a reason for hiding this comment

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

In tryAcquire(), the call to free() is done after the CAS. I think we should build the Holder with a strong reference by default, and only when we successfully CAS to 1 we call free().
In this way the two behaviors (here and in tryAcquire()) are normalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbordet Your change was a little different to this comment, but I think it is good.

…ConcurrentPool'.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet merged commit e800631 into jetty-12.0.x Nov 9, 2023
2 checks passed
@sbordet sbordet deleted the experiment/jetty-12/WeakReference-ConcurrentPool branch November 9, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Jetty 12: leak tracking of Pool entries
3 participants