-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Optimized CircularQueue by using ArrayBlockingQueue with constant time size implementation #1237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Override
public void clearRegistry() {
overriddenInstanceStatusMap.clear();
**recentCanceledQueue.clear();**
**recentRegisteredQueue.clear();**
recentlyChangedQueue.clear();
registry.clear();
}
The highlighted (**) calls are not thread-safe now.
Also ideally this should be an array-based circular buffer as both the old and the new implementations are not cache-friendly and are slow to iterate which is one of their primary uses, but it does not matter in this context much (if at all), I'm ok with this change as it makes perfect sense to me as long as you address the issues.
eureka-core/src/main/java/com/netflix/eureka/registry/AbstractInstanceRegistry.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comments.
I was also thinking about it, and I even have working implementation, but I thought it would be too risky for you to accept it considering low performance difference (I measured only I added commit with additional synchronization and array backed implementation. I also noticed that iteration just moves elements to array list, which is much faster when using collection as a source, especially with efficient
I don't know how to create a benchmark that has proper memory layout to show higher cache hit rate for array backed implementation, but even difference between loop copy - |
Sorry for the delay on this one, I'm giving this full attention now. |
@@ -1278,29 +1280,113 @@ long getCurrentTimeNano() { // for testing | |||
|
|||
} | |||
|
|||
private class CircularQueue<E> extends ConcurrentLinkedQueue<E> { | |||
/* visible for testing */ static class CircularQueue<E> extends AbstractQueue<E> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me but considering you're rolling out a data structure, the amount of unit tests is expected to be much larger, I don't see test cases for [0, 1, even #, odd #] capacity buffers, tests for ops when under capacity, same tests for at capacity, etc. The risk/benefit ratio is just too high.
Based on your benchmarks and iterations so far, it seems like the best way forward is to
- Changed to LinkedList-based as in iteration 1
- Fix
clearRegistry
to be thread safe as you did here - Pre-allocate result lists in
getLastN
methods similar to 3e7a824#diff-f80bacaf7859f75ae25a290673be6b19R174
This would be correct, about the same performance, get rid of unnecessary synchronization, require no additional testing.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it would be simpler to wrap an ArrayBlockingQueue
. Something like:
import java.util.AbstractQueue;
import java.util.Iterator;
import java.util.concurrent.ArrayBlockingQueue;
public class CircularQueue<T> extends AbstractQueue<T> {
private final ArrayBlockingQueue<T> underlying;
CircularQueue(int capacity) {
this.underlying = new ArrayBlockingQueue<>(capacity);
}
@Override
public Iterator<T> iterator() {
return underlying.iterator();
}
@Override
public int size() {
return underlying.size();
}
@Override
public boolean offer(T item) {
while (!underlying.offer(item)) {
underlying.poll();
}
return true;
}
@Override
public T poll() {
return underlying.poll();
}
@Override
public T peek() {
return underlying.peek();
}
// Override for efficiency
@Override
public void clear() {
underlying.clear();
}
@Override
public Object[] toArray() {
return underlying.toArray();
}
}
@brharrington I agree with you, I checked the code and implementation is the same as mine, except locking. And I more believe Doug Lea than myself to implement and test data structures :) Also it looks like with this approach we can remove locking on insertion as this code will be sufficient even under race:
@troshko111 what do you think on this approach? |
This collection is thread-safe and based on the usage in the instance registry, all of the sync code can be removed, i.e. clear, element addition, toArray. |
@troshko111 I updated the PR with suggestions from @brharrington. Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Since
AbstractInstanceRegistry
already have synchronization onrecentRegisteredQueue
andrecentCanceledQueue
it's safe to changeCircularQueue
implementation to extendLinkedList
with constant timesize()
.Benchmark:
Results:
fixes #1236