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.
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 |
clintongormley
added
the
:Search/Percolator
clintongormley
changed the title
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_distancefilter, this can improve percolator query indexing and percolating document execution speed.