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

Cache char[] instances #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jack-pappas
Copy link
Contributor

I've made some small modifications to cache char[] instances in a thread-local variable instead of re-allocating them each time one is needed (e.g., to serialize a DateTime to ISO8601 format). Running the "quick graph" mode of the benchmark tool didn't show any real difference (better or worse), but it seems better to have fewer allocations/GCs if possible to avoid impacting other code in the process.

… to use a thread-local, cached char[] instance when serializing types that need one rather than allocating an instance each time it's needed.
@kevin-montrose
Copy link
Owner

This has been suggested in the past, but I'm hesistant about adding a dynamically sized cache. Jil does allocate some stuff as it's used that isn't reclaimed (because it hangs off of types typically as a static), but once the first hit is paid for the overhead is more or less constant for that type.

A dynamically sized cache to conceivably eat up quite a lot of memory if large object graphs are in play, and that could be pretty bad depending on the app. Offhand, at least the public api on Stack Overflow (a Jil consumer) would be at risk of encountering serious issues.

@jack-pappas
Copy link
Contributor Author

Kevin, did you look at the code in the PR I submitted? The cache is not dynamically-sized, it's just a fixed-length char[] stored in a thread-static field. The cached array is created on-demand so it's only created if it's actually used, and the number of buffers created has an upper bound which is the number of threads in the process.

@kevin-montrose kevin-montrose reopened this Feb 8, 2016
@kevin-montrose
Copy link
Owner

@jack-pappas I thought I had, but clearly I didn't (or I confused myself) - will re-evaluate, my apologies.

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.

None yet

2 participants