You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by ju...@apache.org on 2014/04/28 18:24:05 UTC
svn commit: r1590697 - in
/jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene:
IndexNode.java IndexTracker.java LuceneIndex.java
Author: jukka
Date: Mon Apr 28 16:24:05 2014
New Revision: 1590697
URL: http://svn.apache.org/r1590697
Log:
OAK-1775: Avoid lock contention in IndexTracker.getIndexNode()
Use a ReadWriteLock instead of exclusive synchronization.
Also refactor getIndexNode() into acquireIndexNode() so we can better
track which IndexNode instances are still in use and correctly close
obsolete ones when no longer accessed.
Modified:
jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexNode.java
jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexTracker.java
jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java
Modified: jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexNode.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexNode.java?rev=1590697&r1=1590696&r2=1590697&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexNode.java (original)
+++ jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexNode.java Mon Apr 28 16:24:05 2014
@@ -24,6 +24,8 @@ import static org.apache.jackrabbit.oak.
import java.io.File;
import java.io.IOException;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.apache.jackrabbit.oak.spi.state.ReadOnlyBuilder;
@@ -74,7 +76,7 @@ class IndexNode {
private final IndexSearcher searcher;
- private int refcount = 0;
+ private final ReadWriteLock lock = new ReentrantReadWriteLock();
private boolean closed = false;
@@ -95,27 +97,33 @@ class IndexNode {
return definition;
}
- synchronized IndexSearcher acquireSearcher() {
- checkState(!closed);
- refcount++;
+ IndexSearcher getSearcher() {
return searcher;
}
- synchronized void releaseSearcher() throws IOException {
- refcount--;
- if (closed && refcount == 0) {
- reallyClose();
+ boolean acquire() {
+ lock.readLock().lock();
+ if (closed) {
+ lock.readLock().unlock();
+ return false;
+ } else {
+ return true;
}
}
- synchronized void close() throws IOException {
- closed = true;
- if (refcount == 0) {
- reallyClose();
- }
+ void release() {
+ lock.readLock().unlock();
}
- private void reallyClose() throws IOException {
+ void close() throws IOException {
+ lock.writeLock().lock();
+ try {
+ checkState(!closed);
+ closed = true;
+ } finally {
+ lock.writeLock().unlock();
+ }
+
try {
reader.close();
} finally {
Modified: jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexTracker.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexTracker.java?rev=1590697&r1=1590696&r2=1590697&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexTracker.java (original)
+++ jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexTracker.java Mon Apr 28 16:24:05 2014
@@ -16,10 +16,16 @@
*/
package org.apache.jackrabbit.oak.plugins.index.lucene;
-import static com.google.common.collect.Iterables.addAll;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Predicates.in;
+import static com.google.common.base.Predicates.not;
+import static com.google.common.base.Predicates.notNull;
import static com.google.common.collect.Lists.newArrayList;
import static com.google.common.collect.Lists.newArrayListWithCapacity;
+import static com.google.common.collect.Maps.filterKeys;
+import static com.google.common.collect.Maps.filterValues;
import static com.google.common.collect.Maps.newHashMap;
+import static java.util.Collections.emptyMap;
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NAME;
import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.TYPE_PROPERTY_NAME;
import static org.apache.jackrabbit.oak.plugins.index.lucene.LuceneIndexConstants.TYPE_LUCENE;
@@ -40,6 +46,9 @@ import org.apache.jackrabbit.oak.spi.sta
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+
class IndexTracker {
/** Logger instance. */
@@ -48,9 +57,12 @@ class IndexTracker {
private NodeState root = EMPTY_NODE;
- private final Map<String, IndexNode> indices = newHashMap();
+ private volatile Map<String, IndexNode> indices = emptyMap();
synchronized void close() {
+ Map<String, IndexNode> indices = this.indices;
+ this.indices = emptyMap();
+
for (Map.Entry<String, IndexNode> entry : indices.entrySet()) {
try {
entry.getValue().close();
@@ -58,68 +70,71 @@ class IndexTracker {
log.error("Failed to close the Lucene index at " + entry.getKey(), e);
}
}
- indices.clear();
}
- void update(NodeState root) {
- Editor editor;
- NodeState before;
-
- synchronized (this) {
- before = this.root;
- this.root = root;
-
- List<Editor> editors = newArrayListWithCapacity(indices.size());
- for (Map.Entry<String, IndexNode> entry : indices.entrySet()) {
- final String path = entry.getKey();
- IndexNode index = entry.getValue();
-
- List<String> elements = newArrayList();
- addAll(elements, PathUtils.elements(path));
- elements.add(INDEX_DEFINITIONS_NAME);
- elements.add(index.getName());
- editors.add(new SubtreeEditor(
- new DefaultEditor() {
- @Override
- public void leave(NodeState before, NodeState after) {
- updateIndex(path, after);
- }
- },
- elements.toArray(new String[elements.size()])));
- }
- editor = CompositeEditor.compose(editors);
+ synchronized void update(NodeState root) {
+ Map<String, IndexNode> original = indices;
+ final Map<String, IndexNode> updates = newHashMap();
+
+ List<Editor> editors = newArrayListWithCapacity(original.size());
+ for (Map.Entry<String, IndexNode> entry : original.entrySet()) {
+ final String path = entry.getKey();
+ final String name = entry.getValue().getName();
+
+ List<String> elements = newArrayList();
+ Iterables.addAll(elements, PathUtils.elements(path));
+ elements.add(INDEX_DEFINITIONS_NAME);
+ elements.add(name);
+ editors.add(new SubtreeEditor(new DefaultEditor() {
+ @Override
+ public void leave(NodeState before, NodeState after) {
+ try {
+ // TODO: Use DirectoryReader.openIfChanged()
+ IndexNode index = IndexNode.open(name, after);
+ updates.put(path, index); // index can be null
+ } catch (IOException e) {
+ log.error("Failed to open Lucene index at " + path, e);
+ }
+ }
+ }, elements.toArray(new String[elements.size()])));
}
- // outside the synchronization block to avoid blocking index access
- EditorDiff.process(editor, before, root); // ignore return value
- }
+ EditorDiff.process(CompositeEditor.compose(editors), this.root, root);
+ this.root = root;
- private synchronized void updateIndex(String path, NodeState state) {
- IndexNode index = indices.remove(path);
- try {
- if (index != null) {
- index.close();
- } else {
- return; // this tracker has already been closed
+ if (!updates.isEmpty()) {
+ indices = ImmutableMap.<String, IndexNode>builder()
+ .putAll(filterKeys(original, not(in(updates.keySet()))))
+ .putAll(filterValues(updates, notNull()))
+ .build();
+
+ for (String path : updates.keySet()) {
+ IndexNode index = original.get(path);
+ try {
+ index.close();
+ } catch (IOException e) {
+ log.error("Failed to close Lucene index at " + path, e);
+ }
}
- } catch (IOException e) {
- log.error("Failed to close Lucene index at " + path, e);
}
+ }
- try {
- // TODO: Use DirectoryReader.openIfChanged()
- index = IndexNode.open(index.getName(), state);
- if (index != null) { // the index might no longer exist
- indices.put(path, index);
- }
- } catch (IOException e) {
- log.error("Failed to open Lucene index at " + path, e);
+ IndexNode acquireIndexNode(String path) {
+ IndexNode index = indices.get(path);
+ if (index != null && index.acquire()) {
+ return index;
+ } else {
+ return findIndexNode(path);
}
}
- synchronized IndexNode getIndexNode(String path) {
+ private synchronized IndexNode findIndexNode(String path) {
+ // Retry the lookup from acquireIndexNode now that we're
+ // synchronized. The acquire() call is guaranteed to succeed
+ // since the close() method is also synchronized.
IndexNode index = indices.get(path);
if (index != null) {
+ checkState(index.acquire());
return index;
}
@@ -135,7 +150,11 @@ class IndexTracker {
if (TYPE_LUCENE.equals(node.getString(TYPE_PROPERTY_NAME))) {
index = IndexNode.open(child.getName(), node);
if (index != null) {
- indices.put(path, index);
+ checkState(index.acquire());
+ indices = ImmutableMap.<String, IndexNode>builder()
+ .putAll(indices)
+ .put(path, index)
+ .build();
return index;
}
}
Modified: jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java?rev=1590697&r1=1590696&r2=1590697&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java (original)
+++ jackrabbit/oak/trunk/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndex.java Mon Apr 28 16:24:05 2014
@@ -177,35 +177,39 @@ public class LuceneIndex implements Full
@Override
public double getCost(Filter filter, NodeState root) {
- if (tracker.getIndexNode("/") == null) {
- // unusable index
+ IndexNode index = tracker.acquireIndexNode("/");
+ if (index == null) { // unusable index
return Double.POSITIVE_INFINITY;
}
- FullTextExpression ft = filter.getFullTextConstraint();
- if (ft == null) {
- // no full-text condition: don't use this index,
- // as there might be a better one
- return Double.POSITIVE_INFINITY;
+ try {
+ FullTextExpression ft = filter.getFullTextConstraint();
+ if (ft == null) {
+ // no full-text condition: don't use this index,
+ // as there might be a better one
+ return Double.POSITIVE_INFINITY;
+ }
+ Set<String> relPaths = getRelativePaths(ft);
+ if (relPaths.size() > 1) {
+ LOG.warn("More than one relative parent for query " + filter.getQueryStatement());
+ // there are multiple "parents", as in
+ // "contains(a/x, 'hello') and contains(b/x, 'world')"
+ return new MultiLuceneIndex(filter, root, relPaths).getCost();
+ }
+ String parent = relPaths.iterator().next();
+ if (parent.isEmpty()) {
+ // no relative properties
+ return 10;
+ }
+ // all relative properties have the same "parent", as in
+ // "contains(a/x, 'hello') and contains(a/y, 'world')" or
+ // "contains(a/x, 'hello') or contains(a/*, 'world')"
+ // TODO: proper cost calculation
+ // we assume this will cause more read operations,
+ // as we need to read the node and then the parent
+ return 15;
+ } finally {
+ index.release();
}
- Set<String> relPaths = getRelativePaths(ft);
- if (relPaths.size() > 1) {
- LOG.warn("More than one relative parent for query " + filter.getQueryStatement());
- // there are multiple "parents", as in
- // "contains(a/x, 'hello') and contains(b/x, 'world')"
- return new MultiLuceneIndex(filter, root, relPaths).getCost();
- }
- String parent = relPaths.iterator().next();
- if (parent.isEmpty()) {
- // no relative properties
- return 10;
- }
- // all relative properties have the same "parent", as in
- // "contains(a/x, 'hello') and contains(a/y, 'world')" or
- // "contains(a/x, 'hello') or contains(a/*, 'world')"
- // TODO: proper cost calculation
- // we assume this will cause more read operations,
- // as we need to read the node and then the parent
- return 15;
}
/**
@@ -255,35 +259,36 @@ public class LuceneIndex implements Full
@Override
public String getPlan(Filter filter, NodeState root) {
- IndexNode index = tracker.getIndexNode("/");
+ IndexNode index = tracker.acquireIndexNode("/");
checkState(index != null, "The Lucene index is not available");
-
- FullTextExpression ft = filter.getFullTextConstraint();
- Set<String> relPaths = getRelativePaths(ft);
- if (relPaths.size() > 1) {
- return new MultiLuceneIndex(filter, root, relPaths).getPlan();
- }
- String parent = relPaths.size() == 0 ? "" : relPaths.iterator().next();
- // we only restrict non-full-text conditions if there is
- // no relative property in the full-text constraint
- boolean nonFullTextConstraints = parent.isEmpty();
- String plan = getQuery(filter, null, nonFullTextConstraints, analyzer, index.getDefinition()) + " ft:(" + ft + ")";
- if (!parent.isEmpty()) {
- plan += " parent:" + parent;
+ try {
+ FullTextExpression ft = filter.getFullTextConstraint();
+ Set<String> relPaths = getRelativePaths(ft);
+ if (relPaths.size() > 1) {
+ return new MultiLuceneIndex(filter, root, relPaths).getPlan();
+ }
+ String parent = relPaths.size() == 0 ? "" : relPaths.iterator().next();
+ // we only restrict non-full-text conditions if there is
+ // no relative property in the full-text constraint
+ boolean nonFullTextConstraints = parent.isEmpty();
+ String plan = getQuery(filter, null, nonFullTextConstraints, analyzer, index.getDefinition()) + " ft:(" + ft + ")";
+ if (!parent.isEmpty()) {
+ plan += " parent:" + parent;
+ }
+ return plan;
+ } finally {
+ index.release();
}
- return plan;
}
@Override
public Cursor query(final Filter filter, final NodeState root) {
- final IndexNode index = tracker.getIndexNode("/");
- checkState(index != null, "The Lucene index is not available");
-
FullTextExpression ft = filter.getFullTextConstraint();
Set<String> relPaths = getRelativePaths(ft);
if (relPaths.size() > 1) {
return new MultiLuceneIndex(filter, root, relPaths).query();
}
+
final String parent = relPaths.size() == 0 ? "" : relPaths.iterator().next();
// we only restrict non-full-text conditions if there is
// no relative property in the full-text constraint
@@ -338,14 +343,14 @@ public class LuceneIndex implements Full
* @return true if any document is loaded
*/
private boolean loadDocs() {
- IndexNode indexNode = null;
- IndexSearcher searcher = null;
ScoreDoc lastDocToRecord = null;
+
+ IndexNode indexNode = tracker.acquireIndexNode("/");
+ checkState(indexNode != null);
try {
- indexNode = acquire();
- searcher = indexNode.acquireSearcher();
+ IndexSearcher searcher = indexNode.getSearcher();
Query query = getQuery(filter, searcher.getIndexReader(),
- nonFullTextConstraints, analyzer, index.getDefinition());
+ nonFullTextConstraints, analyzer, indexNode.getDefinition());
TopDocs docs;
if (lastDoc != null) {
docs = searcher.searchAfter(lastDoc, query, LUCENE_QUERY_BATCH_SIZE);
@@ -363,27 +368,14 @@ public class LuceneIndex implements Full
} catch (IOException e) {
LOG.warn("query via {} failed.", LuceneIndex.this, e);
} finally {
- release(indexNode, searcher);
+ indexNode.release();
}
+
if (lastDocToRecord != null) {
this.lastDoc = lastDocToRecord;
}
- return !queue.isEmpty();
- }
- private IndexNode acquire() {
- return tracker.getIndexNode("/");
- }
-
- private void release(IndexNode indexNode, IndexSearcher searcher){
- try {
- if(searcher != null){
- indexNode.releaseSearcher();
- }
- } catch (IOException e) {
- LOG.warn("Error occurred while releasing/closing the " +
- "IndexSearcher", e);
- }
+ return !queue.isEmpty();
}
};
return new LucenePathCursor(itr, settings);