Document that Suppliers.memoizeWithExpiration() starts the countdown when the get() call starts, not when it completes

[gissuebot]

Original issue created by aled.sage on 2012-01-05 at 10:29 AM

---

In jclouds we use the ExpiringMemoizingSupplier (via Suppliers.memoizeWithExpiration). However, there are two (very separate) problems we've seen with this.

Our call to delegate.get() is very expensive - it sometimes takes over 60 seconds on a slow network. When we also configure the duration to 60 seconds, then instead of having 60 seconds between the call returning and the next call, we have 60 seconds between the start of each call. So the returned value is immediately expired. We'd like it to use the returned value for the full duration before calling delegate.get() again. Or at least for that to be a configurable option, if there are conflicting use-cases.

---

Second, when calling delegate.get() it keeps a reference to the old and new values at the same time. Our value can take up a lot of memory, so peak memory consumption is twice as much as required for this (which can cause OOMEs). Perhaps it could set the value to null before calling delegate.get() - depending on the cost of the additional volatile write inside the synchronized block?!

Note: there's some discussion about this at http://groups.google.com/group/jclouds-dev/browse_thread/thread/1aec2817aea53967/1b054f6cc2dfa850

[gissuebot]

Original comment posted by kevinb@google.com on 2012-01-11 at 12:14 AM

---

1. Interesting. I don't think I see a problem with just starting the clock ticking after the element is loaded. Does anyone?
2. Likewise I don't see a particular problem with nulling out the reference. One more volatile write is not likely to be a deal-breaker.

---

Status: Accepted Labels: Type-Enhancement

[gissuebot]

Original comment posted by aled.sage on 2012-01-11 at 12:53 AM

---

Excellent, thanks.

For now in jclouds we've copied and patched the ExpiringMemoizingSupplier code - see https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/util/Suppliers2.java

The two-line change is pretty trivial; it'll be great if we can delete this from our code with the next guava release.

Cheers, Aled

[gissuebot]

Original comment posted by cpovirk@google.com on 2012-01-17 at 08:26 PM

---

Some internally commentary:

Martin: """ resetting the expiration time to after the delegate has returned a value involves an extra call to nanoTime which we would like to avoid. It also seems semantically more correct to start the expiration countdown before the call to delegate.get rather than after - the data may have started going stale near the beginning of that call, and may have been merely delayed "in transit". Most users will have expiration times that are much larger than the time to recompute.

---

Nulling out the old value while computing the new one is more appealing, but here too I'm not sure what the standard practice is when obtaining a value which has already expired - return a stale one or wait for a fresh one to be computed? One would have to be careful not to introduce a race that accidentally returns a null (that would be possible if we naively nulled out value before calling delegate.get()). I would also avoid depending on the monotonicity of nanoTime, which is hard for the JDK/OS to provide reliably, and which we Googlers are known to willingly abandon.  Most obviously we could introduce a NOT_PRESENT sentinel value, but that would at least cost an extra volatile write on every refresh (or resort to Unsafe).

---

I think the current behavior of ExpiringMemoizingSupplier is pretty good for most users. Those who have enormous or expensive caches should either make their caches more fine-grained (as jclouds is doing, I think) or create their own version of ExpiringMemoizingSupplier or use a more full-featured cache, e.g. one that will actively clear the cache when the expiration time elapses without any activity. Which may already exist? """

Me: """ That seems reasonable, and I don't have a strong opinion here.

Arguably the difference is in whether values expire (a) because they are out of date (~become invalid) or (b) because we want to save memory. (a) seems to be a better fit here, as (b) would suggest that the expiration time be bumped every time the value is read. I wonder if our documentation adequately expresses this. In particular, the link to the Wikipedia memoization article feels wrong, as I think of memoization as being about values that would always be the same if they were recomputed. Whatever we can do to improve the documentation here could help, especially since the behavior is inconsistent with Cache. [We are also considering changing Cache's behavior to match.] """

Responses welcome.

---

Status: Triaged

[gissuebot]

Original comment posted by wasserman.louis on 2012-01-20 at 04:52 PM

---

I think that issue 872 is the way to solve this problem: by having a CacheBuilder-like, super-general approach with lots of control over exact expiration semantics. (The refresh-versus-expire distinction in Cache would address this difficulty, no?)

[gissuebot]

Original comment posted by kevinb@google.com on 2012-02-16 at 06:29 PM

---

(No comment entered for this change.)

---

Labels: Package-Base

[gissuebot]

Original comment posted by fry@google.com on 2012-02-16 at 07:17 PM

---

(No comment entered for this change.)

---

Status: Acknowledged

[gissuebot]

Original comment posted by kevinb@google.com on 2012-02-16 at 09:50 PM

---

Personally I would prefer to just document the existing method's behavior rather than try to change it now. I agree with Martin's point that the current behavior is probably fine for the vast majority of users.

[gissuebot]

Original comment posted by kevinb@google.com on 2012-06-22 at 06:16 PM

---

(No comment entered for this change.)

---

Status: Research

[netdpb]

Let's focus this issue on better documenting the current behavior of Suppliers.memoizeWithExpiration(...), and let #872 represent anything more sophisticated.