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

Clean up memory reuse a bit. #9272

Closed
wants to merge 2 commits into from

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Jan 13, 2015

  • don't allow for soft references anymore in the recycler
  • remove some abusive thread locals
  • don't recycle independently float/double and int/long pages, they are the
    same and just interpret bits differently.

 - don't allow for soft references anymore in the recycler
 - remove some abusive thread locals
 - don't recycle independently float/double and int/long pages, they are the
   same and just interpret bits differently.
@@ -273,12 +273,6 @@ public String buildFullName(BuilderContext context) {
}
}

private static final ThreadLocal<List<Field>> FIELD_LIST = new ThreadLocal<List<Field>>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be a different change and we should look at the case where some docs have 1000 fields. Are we sure the auto-growth of the field list is not worth caching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list is per-field so you would need 1000 values for a single field to make it grow to a capacity of 1000. I think it is quite uncommon? I am personally more concerned about the opposite case that you suddenly index a large document and memory is never reclaimed since the list is stored in a ThreadLocal?

Copy link
Contributor

Choose a reason for hiding this comment

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

OH,I see. Misread the code :( .

In that case +1 on removal , it's typically one or two values. Note how the old code initialized it with a capacity of 2 - maybe keep that?

@bleskes
Copy link
Contributor

bleskes commented Jan 13, 2015

I like the paged array part. Left a minor concern regarding the field mappers change.

@jpountz
Copy link
Contributor Author

jpountz commented Jan 13, 2015

@bleskes pushed a new commit

@bleskes
Copy link
Contributor

bleskes commented Jan 13, 2015

LGTM. Thx!

@jpountz jpountz closed this in a56520d Jan 13, 2015
@jpountz jpountz removed the review label Jan 13, 2015
@kimchy
Copy link
Member

kimchy commented Jan 13, 2015

nice!, I love the fact that this frees more memory now..., I wonder if we can back port to 1.5 as well? this is big.

@jpountz
Copy link
Contributor Author

jpountz commented Jan 13, 2015

Reopening for backport.

@jpountz jpountz reopened this Jan 13, 2015
jpountz added a commit that referenced this pull request Jan 14, 2015
 - remove some abusive thread locals
 - don't recycle independently float/double and int/long pages, they are the
   same and just interpret bits differently.

Close #9272
@jpountz
Copy link
Contributor Author

jpountz commented Jan 14, 2015

Merged to 1.x. I just had to leave the support for soft references which are still used by the CacheRecycler (used for facets).

@jpountz jpountz closed this Jan 14, 2015
@clintongormley clintongormley changed the title Internal: clean up memory reuse a bit. Clean up memory reuse a bit. Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants