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

Reuse IndexFieldData instances between percolator queries #6845

Closed

Conversation

Projects
None yet
4 participants
@martijnvg
Copy link
Member

commented Jul 13, 2014

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.

@s1monw

View changes

src/main/java/org/elasticsearch/index/fielddata/IndexFieldDataService.java Outdated
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) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 14, 2014

Contributor

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

@s1monw s1monw removed the review label Jul 14, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2014

left a comment, should this go into 2.0 as well?

@s1monw s1monw added v1.4.0 and removed v1.3.0 labels Jul 14, 2014

@martijnvg martijnvg added the v2.0.0 label Jul 14, 2014

@s1monw s1monw added v1.4.0 and removed blocker labels Jul 14, 2014

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2014

Thanks. I tagged it with 2.0, I'll update this PR, once #6855 is in.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2014

@martijnvg #6855 is pushed

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2014

@s1monw awesome, I will update this PR today.

martijnvg added some commits Jul 13, 2014

Percolator: Cache IndexFieldData instances when loaded directly to re…
…use instances between percolator queries.
@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2014

@s1monw Updated the PR.

@martijnvg martijnvg added the review label Jul 16, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2014

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

@jpountz jpountz removed the review label Jul 17, 2014

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2014

Closing in favour for #7081

@martijnvg martijnvg closed this Jul 29, 2014

@martijnvg martijnvg deleted the martijnvg:bug/percolator_fielddata_access_slow branch May 18, 2015

@clintongormley clintongormley changed the title Percolator: Reuse IndexFieldData instances between percolator queries Reuse IndexFieldData instances between percolator queries Jun 7, 2015

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.