Fix the eval component cache #629
Conversation
| @@ -122,18 +123,7 @@ public void registerSharedNodes(Iterable<DAGNode<Component, Dependency>> nodes) | |||
| * @param node A node that should be cached. | |||
| */ | |||
| public void registerSharedNode(DAGNode<Component, Dependency> node) { | |||
There was a problem hiding this comment.
As long as this is a convienace method wrapper anyways, why not make it varagrs?
There was a problem hiding this comment.
I suppose there isn't a good reason not to, although this method is very rarely used.
|
Newest changes (dfad719) should make it pass the tests. |
| @@ -161,7 +151,7 @@ Object instantiate(@Nonnull DAGNode<Component, Dependency> node) throws Injectio | |||
| entry = cache.get(original); | |||
| } | |||
| if (entry == null) { | |||
| return node; | |||
| entry = new CacheEntry(node); | |||
There was a problem hiding this comment.
I want to check if I understand the impact of this correctly, as this seems to be the important change:
There appear to be two code paths for object sharing through this class. The first uses the register node functions to explicitly ask for cacheing. This does exactly what it says on the tin. (where does this code path get used?)
The second code path is done by asking it to process a graph (or set of nodes) in the past if those nodes were not registered previously you did nothing. (just returned the nodes unmodified). This code change allows these nodes to go through CacheEntries attempted instantiation (if necissary) (where were these nodes otherwise being instantiated if they wern't being instantiated here?)
By doing this you 1) have to make a throwaway cache entry which feels a little weird because it isn't actually entered into the cache from what I can tell
and 2) get to try to load the object from disk in case the only sharing happens between executions.
There was a problem hiding this comment.
You are correct.
Previously, the no-op meant that the node did not get instantiated until test time, since the only build-time instantiation happened through the cache. The cache's interface is kind of broken, but this dummy entry hack lets it actually instantiate nodes so that build-time is really build-time.
There might be an issue here with undesired saving-to-disk. Things should only be saved to disk if they were registered.
|
Before reviewing it I probably should have checked: did you want a code review here? |
mdekstrand
force-pushed
the
feature/elehack-fix-eval-cache
branch
from
dfad719
to
678e35b
Compare
|
I have updated this pull request to fix the little issues, and to not include the fast iteration changes. |
|
Im probably not going to be able to look at this more until monday. |
|
@kluver That's fine. I think I've addressed your concerns, albeit with another ugly hack to fix the problem with my ugly hack. Since tests now pass, I think I'll go ahead and merge (unless you protest). |
Fix the eval component cache and some other improvements.
The eval component cache wasn't instantiating components when it wasn't in heavy use, totally messing up the build vs. eval phase distinctions.
@kluver, this may also have been causing the caching problems experienced by the MovieLens team.