Repository navigation
Make CacheBuilder Cache stats collection optional #863
Description
Original issue created by kimchy on 2012-01-11 at 01:27 PM
It would be great to expose CacheBuilder#disableStats as a public method so we can disable stats completely when its not needed.
Activity
gissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-01-11 at 05:11 PM
Is this actually a performance problem for you?
gissuebot commentedon Oct 31, 2014
Original comment posted by kimchy on 2012-01-11 at 05:15 PM
Yes, it adds up. Also, all my stats are based on LongAdder from jsr166e, and I have a whole stats package built, so would love to be able to plug my own.
gissuebot commentedon Oct 31, 2014
Original comment posted by kimchy on 2012-01-11 at 05:16 PM
Cont: Plug my own, i.e. have my own implementation for StatsCounter.
gissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-01-11 at 08:36 PM
I think it's highly unlikely that we'll provide either of these features, but I should explain why.
With regards to disabling stats, I find it almost impossible to believe that the overhead of maintaining cache stats isn't dominated by hash table lookups, concurrency details, and computing cache values. This isn't a very expensive operation. If you can provide profiling data to support this claim, then please let us know, but we'd need numbers to consider this change.
For the overwhelming majority of Cache users, maintaining stats is either definitely good or doesn't hurt, and correctly maintaining stats isn't a trivial thing. Providing a way to plug your own stats manipulation would tremendously complicate the API, all to provide a feature that is a bad idea for 90% of Cache users. If you absolutely need to do your own stats work, you could probably design a CacheLoader wrapper that'd time cache load operations and the like, or figure out a ForwardingCache-based decorator that'd do what you need.
Status:
TriagedLabels:
package-cachegissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-01-11 at 08:36 PM
(No comment entered for this change.)
Labels: -
package-cache,Package-Cache,Type-Enhancementgissuebot commentedon Oct 31, 2014
Original comment posted by kimchy on 2012-01-11 at 08:48 PM
grr, the usual "prove is problematic and we will fix it" answer. You know the overhead of AtomicLong (writing to volatile field compared to reading in CHM lookup with CHM that hardly changes), and if I can remove it, its great. The get I do is in a very tight loop over a large number of hits. If you want, I can provide a test that shows the overhead of writing to volatile field compared to just reading from one, but honestly, I am now concerned that guava people don't know it...
Besides, the AbstractCache#StatsCounter interface is marked as @Beta, which is public, so I assumed it is planned to be exposed?
gissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-01-11 at 09:06 PM
No, the overhead of AtomicLong is well-known, although I'm not sure we necessarily have to suffer it. What would you say if we used http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/jsr166e/LongAdder.java?view=markup instead? From the doc: "This class is usually preferable to AtomicLong when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control. Under low update contention, the two classes have similar characteristics. But under high contention, expected throughput of this class is significantly higher, at the expense of higher space consumption."
The AbstractCache#StatsCounter interface is already exposed, but I'm not sure we're planning to do anything more with it other than let Cache implementors make use of it.
gissuebot commentedon Oct 31, 2014
Original comment posted by kimchy on 2012-01-11 at 09:35 PM
I pointed to LongAdder in my previous comment, its part of jsr166e which will be included in Java 8, but I personally use it to compute all the stats I do in my app. You can't really use it unless you embed the class in guava, but I really don't understand the resistance to allow to simply disable the stats, even if you don't plan to allow for custom stats collectors.
gissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-01-11 at 09:44 PM
I'd personally like to hear e.g. Fry's opinion here, but here's my logic:
We like to make it as hard as possible for Guava tools to get misused.
For a large majority of Cache users, it's good to have stats around: because if suddenly hit rate goes way down in production, something's catastrophically wrong; because it's a good way to examine system health generally, etc.
If there's an obvious way to turn stats off, some programmers might turn it off even though it's not actually a good idea for them.
I dunno. I'm curious to hear other Guava team members' opinions.
gissuebot commentedon Oct 31, 2014
Original comment posted by kimchy on 2012-01-11 at 09:52 PM
Sorry, don't buy this reasoning. If you don't use the stats in your code (expose them somehow, JMX, rest), then you don't use it. Nobody will go to a live system and dynamically load a class to the JVM that might introspect the Cache in realtime to suddenly check if there are problems based on stats that were never monitored to begin with. If they are monitored / enabled, then fine. Thats the default. If not, let me disable them.
gissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-01-11 at 10:43 PM
I think I'm getting more convinced, but I'd like to hear other opinions.
gissuebot commentedon Oct 31, 2014
Original comment posted by fry@google.com on 2012-01-12 at 04:59 PM
I added disableStats at the suggestion of Tim Peierls, but Kevin suggested that we wait to make it public until anyone actually needed it. While I'm personally onboard with making it public it would be hugely helpful to have a simple Caliper benchmark that demonstrates the performance gain this would provide to your application.
gissuebot commentedon Oct 31, 2014
Original comment posted by kimchy on 2012-01-12 at 06:02 PM
Here is a simplified test of what I do. Basically, I have a mostly read cache (usually size bound) with a tight loop of many iterations. Writing to a volatile field within that tied loop is, well, problematic. https://gist.github.com/1597555.
gissuebot commentedon Oct 31, 2014
Original comment posted by fry@google.com on 2012-01-12 at 06:12 PM
Cool. Would you be up for creating a trivial Caliper benchmark for that? You've already done most of the work, and that would be a huge selling point for this.
gissuebot commentedon Oct 31, 2014
Original comment posted by kevinb@google.com on 2012-01-13 at 05:40 AM
No matter how clear it is that this new CacheBuilder method would benefit you in your case, the bar is very high for adding more methods to CacheBuilder. We would have to be convinced that enough typical users of CacheBuilder would actually notice this issue costing them a significant amount of time (relative to the other expenses of their request), then discover the disableStats() method, and call it. I'm skeptical.
19 remaining items
gissuebot commentedon Oct 31, 2014
Original comment posted by andreou@google.com on 2012-01-31 at 05:15 PM
Doug's multithreaded benchmarks of CHM come to mind, not sure if they are available somewhere. Applying them to Cache should be useful, regardless of this particular issue. A single-threaded test can't illuminate much, and writing more useful benchmarks is a pain.
gissuebot commentedon Oct 31, 2014
Original comment posted by fry@google.com on 2012-04-03 at 02:09 AM
We were wrong here. We're gonna reverse course. Message to guava-discuss forthcoming.
Status:
AcceptedOwner: fry@google.com
gissuebot commentedon Oct 31, 2014
Original comment posted by fry@google.com on 2012-04-06 at 03:05 PM
Fix to disable stats by default will be pushed soon.
Status:
Fixedgissuebot commentedon Oct 31, 2014
Original comment posted by tsunanet on 2012-04-06 at 04:05 PM
Breaking the API is not cool :(
Why didn't you want to add LongAdder to Guava and use that by default (+ a knob in CacheBuilder to allow disabling stats if needed, but leave them on by default)?
gissuebot commentedon Oct 31, 2014
Original comment posted by fry@google.com on 2012-04-06 at 04:08 PM
We also plan to migrate to LongAdder, but that only helps throughput and not latency.
gissuebot commentedon Oct 31, 2014
Original comment posted by wasserman.louis on 2012-04-06 at 04:28 PM
I think the benchmarks demonstrate that turning them off by default is the only long-term solution, even if that causes some one-time pain for users. =/
gissuebot commentedon Oct 31, 2014
Original comment posted by cpovirk@google.com on 2012-04-06 at 04:42 PM
We've cherry picked this into the release 12 branch. It will be in rc2.
Labels:
Milestone-Release12