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
Reuse IndexFieldData instances between percolator queries #6845
Reuse IndexFieldData instances between percolator queries #6845
Conversation
if (fieldData == null) { | ||
// By caching IFD instances we prevent that we end up with equal to number of percolator instances | ||
// `loadedFieldData` can't be used b/c we forcefully always use a different IndexFieldDataCache impl | ||
synchronized (loadedDirectFieldData) { |
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 effectivly means there is only one field loading concurrently on this service since you are locking on the loadedDirectFieldData
I am 100% certain about all the implications but I'm 99% sure this is the wrong way to do that. If you want to prevent a single field from loading twice at the same time we have a nice datastructure for this called KeyedLock
that you can use like this"
private KeyedLock<String> directLoadingLock = new KeyedLock<>();
//...
final String key = fieldNames.indexName();
directLoadingLock.acquire(key);
try {
// load your stuff
} finally {
directLoadingLock.release(key)
}
that way you can just remove all your synchronizaion
left a comment, should this go into 2.0 as well? |
Thanks. I tagged it with 2.0, I'll update this PR, once #6855 is in. |
@martijnvg #6855 is pushed |
@s1monw awesome, I will update this PR today. |
…use instances between percolator queries.
@s1monw Updated the PR. |
It would be nice to try to share more code between getForField and getForFieldDirect. I can easily see them getting out of sync otherwise. Other than that, LGTM |
Closing in favour for #7081 |
This is an improvement that came out of #6806.
Instead of each percolator query having its own IndexFieldData instance, it is better to reuse the same instance between percolator queries. For example in case when many percolator queries use the same
geo_distance
filter, this can improve percolator query indexing and percolating document execution speed.