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

LUCENE-9955: Reduced state of stored fields readers. #137

Closed
wants to merge 4 commits into from

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented May 12, 2021

This removes most state from stored fields readers.

#10994

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find, LGTM.

@rmuir
Copy link
Member

rmuir commented May 12, 2021

it would be great to open a followup "wish" issue at least to think about ways we might remove the stored fields and term vectors threadlocals completely? The difficult part is figuring out how to make the api easy, but i feel like it should be possible, e.g. something minimally invasive like:

// old
Document d1 = reader.doc(doc1);
Document d2 = reader.doc(doc2);
// new
var storedfields = reader.getStoredFields();
Document d1 = storedfields.doc(doc1);
Document d2 = storedfields.doc(doc2);

then the storedfields would just be garbage-collected normally like any other lucene index api. So you'd still prevent clone()/shared dictionary/whatever resources from happening per-document, but they'd happen per-query (is this good enough?)

Anyway, just worth a thought for the future. I hate making these apis difficult to use, but at the same time I don't like how trappy the threadlocals can be (especially if you arent using fixed threadpools etc).

@jpountz
Copy link
Contributor Author

jpountz commented May 14, 2021

Agreed we should look into this! I opened https://issues.apache.org/jira/browse/LUCENE-9959.

@rmuir
Copy link
Member

rmuir commented Jul 19, 2021

-1 to this code change which started all the new public classes/renaming problems

mikemccand pushed a commit to mikemccand/lucene that referenced this pull request Sep 3, 2021
@jpountz jpountz closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants