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

Projects
None yet
5 participants
@s1monw
Copy link
Contributor

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.

@s1monw s1monw added review labels May 16, 2014

@nik9000

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2014

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

@s1monw

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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...

Remove SoftReferences from StreamInput/StreamOutput
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

This comment has been minimized.

Copy link
Member

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 s1monw:cleanup_stream_in_out branch May 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.