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

currentTimeMillis() was returning microsecondonds #40

Merged
merged 1 commit into from
Dec 14, 2015

Conversation

rogerkl
Copy link

@rogerkl rogerkl commented Dec 14, 2015

should divide by 1000000 (>> 20 => 1048576) not 1000 (>> 10 => 1024)

should divide by 1000000 (>> 20 => 1048576) not 1000 (>> 10 => 1024)
@ben-manes ben-manes merged commit 22a0e83 into ben-manes:master Dec 14, 2015
@ben-manes
Copy link
Owner

Thanks for this. Do you want me to release v2.0.3 now with this fix?

I hope to profile the JCache adapters over the holidays or in early January. The spec makes it hard to be performance centric, but I think I can at least get rid of some of the redundant native calls for the time. At the time I focused on the TCK and trying to design an event dispatcher that would be efficient.

@ben-manes
Copy link
Owner

@rogerkl I reviewed the time usages and fixed the statistics MXBean which was misreporting the average operation times. As you know it passes the TCK, but unfortunately that's not good at catching these silly mistakes. I can release whenever you want, e.g. soon or wait in case you find other issues we should fix.

@rogerkl
Copy link
Author

rogerkl commented Dec 14, 2015

Thanks @ben-manes, It's not critical to have a release now, as we're not using caffeine in production now, so I could do some more testing. Our jcache usage is not very performance critical for the moment, we're using @CacheResult annotation (with guice) to avoid some expensive lookups, but I'll be on the lookout for more bugs :-)

@ben-manes
Copy link
Owner

Thanks again :-)

Please be aware that @CacheResult does not perform the computation atomically. This is a requirement in the spec, which dictates that a racy get then compute then put sequence must be performed (see annotation's JavaDoc). This means the usages are vulnerable to the thundering herds problem.

@ben-manes
Copy link
Owner

@rogerkl I profiled the adapter and fixed a lot of low hanging fruit. This raised read performance from 20M to 135M / s and writes from 9M to 35M / s. That's on a simple profiler hook benchmark, not JMH, so its observational only. I only focused on get & put, and applied the same fixed across other methods.

The main culprit was the slow calls to get the current time, which is most often unnecessary. That's a slow operation, especially when being thrashed. Its more an issue in micro-benchmarks than systems.

The writes are still slower than I'd like (should be closer to 65M), but it has to use a slower compute path. There may be some low hanging fruit I missed too. Since caches are read heavy its probably okay.

I think a release is probably worth doing soon so that the JCache adapter is more production worthy.

@rogerkl
Copy link
Author

rogerkl commented Dec 18, 2015

Sounds good @ben-manes, thanks. I will check it out. Keep up the good work.

@ben-manes
Copy link
Owner

Released 2.0.3

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.

2 participants