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

1.x Cache string values to reduce memory footprint. #434

Merged
merged 2 commits into from
Feb 25, 2015

Conversation

tbak
Copy link

@tbak tbak commented Feb 24, 2015

No description provided.

@cloudbees-pull-request-builder

NetflixOSS » eureka » eureka-pull-requests #114 SUCCESS
This pull request looks good

/**
* @author Tomasz Bak
*/
public class StringCache {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you are not using String.intern() simply?

Copy link
Author

Choose a reason for hiding this comment

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

Using String.intern() was technically forbidden in the past for string pool usage. This implementation of StringCache follows xstream's StringConverter which I tried to use, but since we have custom serializer it is never invoked.
I checked again, and actually in Java7 the String.intern() implementation is fixed, and it is better alternative than having own string pools, but unless running on 7u40, it requires setting -XX:StringTableSize=N to proper value (before 7u40 default = 1009, later default = 60013 which is better but we might need even higher value). Check this article: http://java-performance.info/string-intern-in-java-6-7-8/
Given that we do not control which Java7 JVM versions clients run, and we cannot enforce proper JVM configuration, sticking to own String pool implementation seems to be better option short term. Medium/long term, if we want to make similar optimization in Eureka 2.x I would go for String.intern.

@qiangdavidliu
Copy link
Contributor

Rather than implementing a StringCache independent of xstream, can't we just register a StringConverter with high priority when we register the specific converters for the JsonXStream and XmlXStream?

We may also want to look at implementing an alternative to the default StringConverter similar to http://javadoc.jenkins-ci.org/hudson/util/HeapSpaceStringConverter.html.

@tbak
Copy link
Author

tbak commented Feb 24, 2015

I tried to setup StringConverter, and my custom converter implementation, but it never got triggered. Our custom serializers do not delegate processing further once they got basic types. If you know how to enforce delegation to StringConverter it would be much nicer implementation.

@cloudbees-pull-request-builder

NetflixOSS » eureka » eureka-pull-requests #122 SUCCESS
This pull request looks good

tbak pushed a commit that referenced this pull request Feb 25, 2015
1.x Cache string values to reduce memory footprint.
@tbak tbak merged commit 5ab86b9 into Netflix:master Feb 25, 2015
@tbak tbak deleted the refactorings/string_cache branch February 27, 2015 22:14
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.

4 participants