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

Reduce locking in FieldCacheImpl::Cache::Get #130

Open
bongohrtech opened this issue Jul 12, 2019 · 3 comments
Open

Reduce locking in FieldCacheImpl::Cache::Get #130

bongohrtech opened this issue Jul 12, 2019 · 3 comments

Comments

@bongohrtech
Copy link
Owner

We noticed a lot of contention in FieldCacheImpl::Cache::Get (our queries use a lot of query time joins + sorting, so we hit the field cache a lot).

We use a SearcherManager with warm-up queries to populate the field cache so we would expect it to be initialized in most cases before we hit it for actual requests.

The implementation seems to lock even for the happy path (when everything's already initialized). This seems like a by-product of the choice of data structures (the underlying WeakDictionary, WeakHashMap etc are not threadsafe) and so the locking is required in case the dictionary gets resized.

Ideally we could be using thread-safe data structures and only lock when initializing the data.

JIRA link - [LUCENENET-610] created by sthmathew
@bongohrtech
Copy link
Owner Author

Thanks for the report. Could you please provide a reproduction case, either in the form of an integration test or a console app?

by nightowl888

@bongohrtech
Copy link
Owner Author

While having a thread safe collection would be ideal, that is not how it was implemented in Lucene. However, I took a look at the implementation of WeakDictionary and it is dissimilar to that of the java.util.WeakHashMap that was used in the OpenJDK. In particular the CleanIfNeeded() method moves the entries from one dictionary instance to another every time it is called, which is less than ideal.

According to this StackOverflow answer, it may be possible to use a ConditionalWeakTable as a replacement.

However, before it is attempted, it would be nice to have a reproduction case so it is possible to see whether we are indeed solving the issue or making it worse. Could you please provide one?

by nightowl888

@bongohrtech
Copy link
Owner Author

We have replaced WeakDictionary with ConditionalWeakTable in Lucene.NET 4.8.0-beta00007, but some of the APIs of ConditionalWeakTable that Lucene.NET requires are only available on .NET Standard 2.1.

If you are using a platform that supports .NET Standard 2.1, could you please check out whether this change resolves the issue you are experiencing?

by nightowl888

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant