You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jena.apache.org by ajs6f <gi...@git.apache.org> on 2015/05/07 21:11:41 UTC
[GitHub] jena pull request: Relying more on Guava impl for caching
GitHub user ajs6f opened a pull request:
https://github.com/apache/jena/pull/57
Relying more on Guava impl for caching
The idea here is to simplify Jena's code by relying a little more on the Guava implementation for caching.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ajs6f/jena Java8StuffForCaching
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/jena/pull/57.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #57
----
commit 68e99dcb38f38a3ecd618962a94a6ccc2eb40ff3
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-07T19:08:04Z
Relying more on Guava impl
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29957586
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
No problem. It was definitely to avoid `null`-checking, because `BlockMgrCache` sprouted all kinds of `null`-checks when I killed `Cache0`. As I commented there, I would like to rewrite `BlockMgrCache` to use `Optional` but I'm not yet confident enough that I understand it well enough, and I'm not yet familiar enough with the test framework to understand how well-covered `BlockMgrCache` is and how careful I need to be around it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-100894484
Sounds good to me!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29939141
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
If keys K1 and K2 have the same short hash, then one adding K2 overwrites k1 in CacheSimple but in a java Map, you end up with two entries K1 and K2, with internal reorganisation as needed and the Map is bigger. CacheSimple is a sort of fixed size map.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938565
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
A Map grows as more, different, keys are added whereas the existing CacheSimple is a fixed size slot replacement cache with the property that it never resizes.
CacheSimple is a fast cache, traded against a poorer than LRU replacement/eviction policy of same-hash slot replacement. Using a Map changes its characteristics.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29955075
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
I've made changes to `BlockMgrCache` sufficient to permit the removal of `Cache0` from the codebase, but I'd much appreciate a careful review here. Also, I have to admit I think this new code (with all the `null`-checks) to be pretty ugly. I'd like to rewrite it using `Optional`, but I'd first like to be sure that my basic understanding is correct.
P.S. Actually, I'd like to make `Cache::getIfPresent` return `Optional`. That would really eliminate a lot of `null`-checking.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-100901958
Oh, cool. Yeah, there's a number of message-emitting sources here, eh? Asynchronous transactions are tricky. {grin}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938061
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -43,7 +43,7 @@
/** One slot cache */
public static <Key, Value> Cache<Key, Value> createOneSlotCache() {
- return new Cache1<>() ;
+ return new CacheGuava<>(1) ;
}
--- End diff --
CacheGuava is a bit heavy for a one-slot cache. While Cahce1 may not be used currently, if it were, then keeping it as simple, small implementation is better. Proposal: Keep Cache1 as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/jena/pull/57
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-100894301
I fixed the bug while I was looking at it :-) I'll review and try out the current state and merge it if there are no further comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938256
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -33,7 +33,7 @@
/** Create a null cache */
public static <Key, Value> Cache<Key, Value> createNullCache() {
- return new Cache0<>() ;
+ return new CacheGuava<>(0) ;
}
--- End diff --
You mean see to it that `Cache0` is not used at all, then instead of reimpling it with Guave, just get rid of it entirely?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29957286
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
I hadn't realized that Cache0 was used. My bad.
The use in BlockMgrCache for a "not a cache really" avoid null checking or some such in BlockMgrCache.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r30033649
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
Up to the presence of that bug in `CacheSimple`: see my comment there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29939353
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
How about using [`LinkedHashMap`'s ability to keep fixed-size?](http://docs.oracle.com/javase/8/docs/api/java/util/LinkedHashMap.html#removeEldestEntry-java.util.Map.Entry-)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29957731
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
`Cache::getIfPresent` returning Optional means that the calling code ends up checking Optional rather than null. Looking in TDB, for example, it's no gain, in fact it's a loss because of the additional object creation.
The reason is that these aren't in streams where Optional skipping is natural.
The node cache is performance sensitive and uses getIfPresent so changes here might have undesirable effects.
What would be good is looking at the unrelated cache code in jena-core and seeing if it can now be consolidated (including using LRU caching which IIRC it does not).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938058
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -33,7 +33,7 @@
/** Create a null cache */
public static <Key, Value> Cache<Key, Value> createNullCache() {
- return new Cache0<>() ;
+ return new CacheGuava<>(0) ;
}
--- End diff --
Cache0 is really just a "complete the set" implementation. If any use as a dummy, the weight of CacheGuava might not be worth it. Proposal: just remove Cache0 (it does not fit with getOrFill anyway).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938812
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -33,7 +33,7 @@
/** Create a null cache */
public static <Key, Value> Cache<Key, Value> createNullCache() {
- return new Cache0<>() ;
+ return new CacheGuava<>(0) ;
}
--- End diff --
Yes, hooray for source control. {grin} I will adjust this PR accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] jena pull request: Relying more on Guava impl for caching
Posted by Claude Warren <cl...@xenei.com>.
Why would you not fix the bug?
On Mon, May 11, 2015 at 1:33 PM, ajs6f <gi...@git.apache.org> wrote:
> Github user ajs6f commented on a diff in the pull request:
>
> https://github.com/apache/jena/pull/57#discussion_r30033794
>
> --- Diff:
> jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
> @@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int
> writeSlots, final BlockMgr blockMgr)
> {
> --- End diff --
>
> Wait-- that comment you made about a bug seems to be gone. I guess the
> question I'm asking is whether we want to include `TestCacheSimple` and fix
> the bug, or leave out `TestCacheSimple` and just mark the bug.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
--
I like: Like Like - The likeliest place on the web
<http://like-like.xenei.com>
LinkedIn: http://www.linkedin.com/in/claudewarren
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r30033794
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
Wait-- that comment you made about a bug seems to be gone. I guess the question I'm asking is whether we want to include `TestCacheSimple` and fix the bug, or leave out `TestCacheSimple` and just mark the bug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938759
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -33,7 +33,7 @@
/** Create a null cache */
public static <Key, Value> Cache<Key, Value> createNullCache() {
- return new Cache0<>() ;
+ return new CacheGuava<>(0) ;
}
--- End diff --
Yes. It would not be hard to bring it back!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-100955353
One last thing: it seems like a good idea to me to put a note in `CacheSimple` about the slot-replacement and fixed-size qualities that are demanded there. Or maybe instead to add a test to `TestCacheSimple` to indicate the slot-replacement requirement. Would you like me to do that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-101007860
Good suggestion. I'll add that as I do #59.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29939173
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -43,7 +43,7 @@
/** One slot cache */
public static <Key, Value> Cache<Key, Value> createOneSlotCache() {
- return new Cache1<>() ;
+ return new CacheGuava<>(1) ;
}
--- End diff --
Okay, `Cache1` stays with us. I'll adjust this PR to that effect.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-101004445
Cool. Maybe there should be a comment on `CacheSetImpl` explaining why it must "inherit" the eviction policy from the wrapped `Cache`. It's not obvious to someone who doesn't know how it's being used downstream in TDB.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] jena pull request: Relying more on Guava impl for caching
Posted by Claude Warren <cl...@xenei.com>.
I replied before my system picked up the previous 2 notes. No problems
here.
On Mon, May 11, 2015 at 2:13 PM, ajs6f <gi...@git.apache.org> wrote:
> Github user ajs6f commented on the pull request:
>
> https://github.com/apache/jena/pull/57#issuecomment-100901273
>
> Because the comment disappeared-- clearly something else was going on,
> as @afs later indicated he already fixed the bug. What is the problem?
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
--
I like: Like Like - The likeliest place on the web
<http://like-like.xenei.com>
LinkedIn: http://www.linkedin.com/in/claudewarren
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-100901273
Because the comment disappeared-- clearly something else was going on, as @afs later indicated he already fixed the bug. What is the problem?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29957143
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
Okay, I'll back out the changes to `CacheSimple`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938974
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -43,7 +43,7 @@
/** One slot cache */
public static <Key, Value> Cache<Key, Value> createOneSlotCache() {
- return new Cache1<>() ;
+ return new CacheGuava<>(1) ;
}
--- End diff --
It's not used (if it is used elsewhere, then as it's not API, reasonable change is OK). The cache interface is useful though and `Supplier` does not cover the `getOrFill` pattern. It also allows a switch from `Cache1` to a bigger, better cache as the interface is the same.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29941387
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -33,7 +33,7 @@
/** Create a null cache */
public static <Key, Value> Cache<Key, Value> createNullCache() {
- return new Cache0<>() ;
+ return new CacheGuava<>(0) ;
}
--- End diff --
Hm. There is one use of `Cache0`. It's indirectly, via `CacheFactory`, and it occurs in `BlockMgrCache` at [line 62](https://github.com/apache/jena/blob/master/jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java#L62). I'm not sure enough as to `BlockMgrCache`'s responsibilities to feel comfortable rewiring it to not require the existence of a `BlockMgrCache.readCache` without your go-ahead. I would feel better if I better understood the comment "Caches are related so we can't use a Getter for cache management." Does this refer to `readCache` and `BlockMgrCache.writeCache`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938434
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/CacheFactory.java ---
@@ -43,7 +43,7 @@
/** One slot cache */
public static <Key, Value> Cache<Key, Value> createOneSlotCache() {
- return new Cache1<>() ;
+ return new CacheGuava<>(1) ;
}
--- End diff --
Shall I check and see whether `Cache1` is really in use? It tastes to me like memoization, which might be better supplied by some other means in Java 8, e.g via statics that use [Supplier](http://docs.oracle.com/javase/8/docs/api/java/util/function/Supplier.html).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29995826
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
CacheSimple has a plain old, fashioned bug in maintaining the size!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29958256
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
Okay, I did notice that stuff (in `jena-core`) but I wanted to be sure I thoroughly understood this `jena-base` stuff before tackling it. I'm glad I did, because I clearly did _not_ understand these classes well enough to shift things onto them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29946244
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
As you can see, I've added this change and also a test to demonstrate that `CacheSimple` now has the required fixed-size behavior.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938052
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/Cache1.java ---
@@ -1,127 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.jena.atlas.lib.cache;
-
-import java.util.Iterator ;
-import java.util.Objects;
-import java.util.concurrent.Callable ;
-import java.util.function.BiConsumer;
-
-import org.apache.jena.atlas.iterator.SingletonIterator ;
-import org.apache.jena.atlas.lib.Cache ;
-
-/** A one-slot cache.*/
-public class Cache1<K, V> implements Cache<K,V>
-{
--- End diff --
Proposal: keep this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29958374
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
If this whole class is only for 32-bit operation, then I reckon maybe I should put `Cache0` back in and move on. It doesn't seem like a good place to invest time, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29957079
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
LinkedHashMap implement an LRU policy and is what the old CacheLRU used. We swapped that to CacheGuava and removed CacheLRU. Using LinkedHashMap again means we have two LRU caches.
CacheSimple has a specific policy of slot replacement should that be wanted. We can leave it be and decide whether to delete it later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on the pull request:
https://github.com/apache/jena/pull/57#issuecomment-101003455
Thank you. Merged into code base, and reflecting the conversion here: Keep Cache0 (avoid nulls!) and keep the general CacheSet over a Cache due to otherwise changing the eviction policy in TDB missing nodes cache.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29958189
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
Cache0 is the `Optional` of caches :-)
`BlockMgrCache` is mainly used for 32-bit TDB only - 64-bit uses memory mapped file and does not layer a `BlockMgrCache` on top.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29958012
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
Just to be clear, am I correct to be thinking that a `CacheSimple` should never expand beyond the size of its constructor argument? I'm asking because that's exactly what my new test checked, and it passes for my `LinkedHashMap` impl, but not for the original. I build a `CacheSimple<Integer>(5)` and put 10 things in it, mapped from 10 different keys each to the same value, and then its `size()` is 10 when it should only be 5. Or am I misunderstanding the policy here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29938708
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/cache/CacheSimple.java ---
@@ -37,120 +37,81 @@
public class CacheSimple<K,V> implements Cache<K,V>
{
- private final V[] values ;
- private final K[] keys ;
- private final int size ;
- private int currentSize = 0 ;
- private BiConsumer<K,V> dropHandler = null ;
+ private Map<K,V> internalCache;
+
--- End diff --
I did not understand that-- I will rework this PR with attention paid to that point and to whatever answer you give about `Cache0` and `Cache1`. Thanks for the attention!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Relying more on Guava impl for caching
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/57#discussion_r29995830
--- Diff: jena-tdb/src/main/java/org/apache/jena/tdb/base/block/BlockMgrCache.java ---
@@ -58,10 +58,7 @@ private BlockMgrCache(int readSlots, int writeSlots, final BlockMgr blockMgr)
{
--- End diff --
The current clearing up makes a good checkpoint.
This is ready to integrate now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---