You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by scubafuchs <gi...@git.apache.org> on 2017/04/19 19:17:36 UTC
[GitHub] accumulo pull request #251: ACCUMULO-4626 added weak reference value map for...
GitHub user scubafuchs opened a pull request:
https://github.com/apache/accumulo/pull/251
ACCUMULO-4626 added weak reference value map for evictions from block\u2026
\u2026 caches
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/scubafuchs/accumulo ACCUMULO-4626
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/251.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 #251
----
commit cd34462a24ae25425f8de7137b9cdc8eff84bc5a
Author: Adam Fuchs <ad...@sqrrl.com>
Date: 2017-04-19T19:16:14Z
ACCUMULO-4626 added weak reference value map for evictions from block caches
----
---
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] accumulo pull request #251: ACCUMULO-4626 added weak reference value map for...
Posted by ben-manes <gi...@git.apache.org>.
Github user ben-manes commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/251#discussion_r113367686
--- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java ---
@@ -48,12 +52,13 @@
private final Cache<String,Block> cache;
private final Policy.Eviction<String,Block> policy;
private final ScheduledExecutorService statsExecutor;
+ private final Map<String,CacheEntry> weakEvictionMap = new MapMaker().weakValues().makeMap();
public TinyLfuBlockCache(long maxSize, long blockSize) {
cache = Caffeine.newBuilder().initialCapacity((int) Math.ceil(1.2 * maxSize / blockSize)).weigher((String blockName, Block block) -> {
int keyWeight = ClassSize.align(blockName.length()) + ClassSize.STRING;
return keyWeight + block.weight();
- }).maximumWeight(maxSize).recordStats().build();
+ }).maximumWeight(maxSize).recordStats().removalListener((String key, Block block, RemovalCause cause) -> weakEvictionMap.put(key, block)).build();
--- End diff --
It is called after, asynchronously. I think using a CacheWriter would be preferable, as it is called within the atomic operation.
---
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] accumulo pull request #251: ACCUMULO-4626 added weak reference value map for...
Posted by ben-manes <gi...@git.apache.org>.
Github user ben-manes commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/251#discussion_r113368843
--- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java ---
@@ -48,12 +52,13 @@
private final Cache<String,Block> cache;
private final Policy.Eviction<String,Block> policy;
private final ScheduledExecutorService statsExecutor;
+ private final Map<String,CacheEntry> weakEvictionMap = new MapMaker().weakValues().makeMap();
--- End diff --
FYI, Caffeine also supports weak values so you could use two instances. It has better concurrency and creates less garbage, whereas Guava's has slightly better memory efficiency due to forking the hash table.
---
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] accumulo pull request #251: ACCUMULO-4626 added weak reference value map for...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/251#discussion_r112977010
--- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java ---
@@ -48,12 +52,13 @@
private final Cache<String,Block> cache;
private final Policy.Eviction<String,Block> policy;
private final ScheduledExecutorService statsExecutor;
+ private final Map<String,CacheEntry> weakEvictionMap = new MapMaker().weakValues().makeMap();
public TinyLfuBlockCache(long maxSize, long blockSize) {
cache = Caffeine.newBuilder().initialCapacity((int) Math.ceil(1.2 * maxSize / blockSize)).weigher((String blockName, Block block) -> {
int keyWeight = ClassSize.align(blockName.length()) + ClassSize.STRING;
return keyWeight + block.weight();
- }).maximumWeight(maxSize).recordStats().build();
+ }).maximumWeight(maxSize).recordStats().removalListener((String key, Block block, RemovalCause cause) -> weakEvictionMap.put(key, block)).build();
--- End diff --
Do you know if the removal listener is called before or after the entry is removed from the 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] accumulo pull request #251: ACCUMULO-4626 added weak reference value map for...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/251#discussion_r112983687
--- Diff: core/src/test/java/org/apache/accumulo/core/file/blockfile/cache/TestWeakEvictionLruBlockCache.java ---
@@ -0,0 +1,54 @@
+/*
+ * 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.accumulo.core.file.blockfile.cache;
+
+import static org.junit.Assert.assertTrue;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+
+import org.junit.Test;
+
+public class TestWeakEvictionLruBlockCache {
--- End diff --
Can a similar test be added for TinyLfu 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] accumulo pull request #251: ACCUMULO-4626 added weak reference value map for...
Posted by ben-manes <gi...@git.apache.org>.
Github user ben-manes commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/251#discussion_r113368473
--- Diff: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java ---
@@ -68,7 +73,10 @@ public long getMaxSize() {
@Override
public CacheEntry getBlock(String blockName) {
- return cache.getIfPresent(blockName);
+ CacheEntry ce = cache.getIfPresent(blockName);
+ if (ce != null)
+ return ce;
+ return weakEvictionMap.get(blockName);
--- End diff --
You might prefer to promote the entry using a CacheLoader. Otherwise your recovery of a premature eviction would still allow the entry to be collected eventually.
---
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.
---