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

Remove SoftReferences from StreamInput/StreamOutput #6208

Merged
merged 1 commit into from May 16, 2014

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented May 16, 2014

We try to reuse character arrays and UTF8 writers with softreferences.
SoftReferences have negative impact on GC and should be avoided in
general. Yet in this case it can simply replaced with a per-stream
Bytes/CharsRef that is thread local and has the same lifetime as the
stream.

@nik9000
Copy link
Member

nik9000 commented May 16, 2014

I've actually been looking into GC issues lately on my cluser. Soft references account for a pretty small amount of the young gen pause time. Weak references account for about 1/8th of though. Still, no reason not to do this.

@rmuir
Copy link
Contributor

rmuir commented May 16, 2014

Why does StreamOutput write with standard UTF-8 and StreamInput read with modified UTF-8? These are incompatible for some unicode characters...

@s1monw
Copy link
Contributor Author

s1monw commented May 16, 2014

@rmuir wait that is completely unrelated to this issue no? Can you open a followup?

@s1monw
Copy link
Contributor Author

s1monw commented May 16, 2014

Why does StreamOutput write with standard UTF-8 and StreamInput read with modified UTF-8? These are incompatible for some unicode characters...

@rmuir that is not entirely true the write part that I fixed is writeText and read part is readString the corresponding writeString also writes modified UTF-8. Not saying we should move to standard unicode

@rmuir
Copy link
Contributor

rmuir commented May 16, 2014

your right, as long as writeText is always paired with readText and vice versa, it will be ok. but this is dangerous, because bugs could sit latent (and simple tests would not find them). so some care should be taken to reduce this risk...

We try to reuse character arrays and UTF8 writers with softreferences.
SoftReferences have negative impact on GC and should be avoided in
general. Yet in this case it can simply replaced with a per-stream
Bytes/CharsRef that is thread local and has the same lifetime as the
stream.
@kimchy
Copy link
Member

kimchy commented May 16, 2014

+1, it cleans the code. I am less concerned about the SoftReference aspect, since its a static thread local.

@s1monw s1monw merged commit bf22df7 into elastic:master May 16, 2014
@s1monw s1monw removed the review label May 16, 2014
@s1monw s1monw deleted the cleanup_stream_in_out branch May 16, 2014 19:09
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

5 participants