Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@ public void invalidate() {
nodeRef = null;
relPath = null;
state = STATE_INITIAL;
// Invalidate the cached namespace prefixes for this aggregate
mgr.invalidateAggregatePrefixes(path);
}

public Aggregate getParent() {
Expand Down Expand Up @@ -410,7 +412,7 @@ public String[] getNamespacePrefixes() {
}

public String getNamespaceURI(String prefix) throws RepositoryException {
return mgr.getNamespaceURI(prefix);
return mgr.getCachedNamespaceURI(prefix);
}

public Collection<Property> getBinaries() {
Expand Down Expand Up @@ -610,6 +612,8 @@ private void addNamespace(Set<String> prefixes, String name) throws RepositoryEx
String pfx = name.substring(0, idx);
if (!prefixes.contains(pfx)) {
prefixes.add(pfx);
// Cache the prefix in the manager to avoid repeated JCR lookups
mgr.cacheNamespacePrefix(pfx);
}
}
}
Expand All @@ -623,6 +627,14 @@ private void addNamespacePath(Set<String> prefixes, String path) throws Reposito

private void loadNamespaces() {
if (namespacePrefixes == null) {
// Check if this aggregate's namespaces are already cached
String[] cachedPrefixes = mgr.getCachedAggregatePrefixes(path);
if (cachedPrefixes != null) {
log.debug("Using cached namespace prefixes for '{}': {}", path, cachedPrefixes);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, this code is never reached when running the filevault test suite.

We need a test case that actually covers this case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, that code is never reached. Sonar agrees.

It would only help if the same path is scanned multiple times. If that can happen, we should be able to write a test.

namespacePrefixes = cachedPrefixes;
return;
}

if (log.isDebugEnabled()) {
log.trace("loading namespaces of aggregate {}", path);
}
Expand All @@ -635,6 +647,9 @@ private void loadNamespaces() {
loadNamespaces(prefixes, "", getNode());
namespacePrefixes = prefixes.toArray(new String[prefixes.size()]);

// Cache the discovered prefixes for this aggregate path
mgr.cacheAggregatePrefixes(path, namespacePrefixes);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this code correctly, it stores this String-Array of namespacePrefixes in a global cache, per path. But that also means, that there is benefit only in the case that loadNamespaces() is called at least twice (either as part of the same aggregate or in a different one). Under what circumstances would that be helpful then?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added bounded lru cache


// set if and only if in DEBUG level
if (start >= 0) {
Duration duration = Duration.ofNanos(System.nanoTime() - start);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

import org.apache.jackrabbit.vault.fs.api.AggregateManager;
import org.apache.jackrabbit.vault.fs.api.Aggregator;
Expand Down Expand Up @@ -85,6 +86,11 @@ public class AggregateManagerImpl implements AggregateManager {
*/
private static final String DEFAULT_NODETYPES = "" + "org/apache/jackrabbit/vault/fs/config/nodetypes.cnd";

/**
* default logger
*/
private static final Logger log = LoggerFactory.getLogger(AggregateManagerImpl.class);

/**
* the repository session for this manager
*/
Expand Down Expand Up @@ -124,6 +130,18 @@ public class AggregateManagerImpl implements AggregateManager {
*/
private final Set<String> nodeTypes = new HashSet<String>();

/**
* Cache of namespace prefixes to URIs. This cache is shared across all aggregates
* to avoid expensive JCR tree traversals for namespace discovery.
*/
private final ConcurrentHashMap<String, String> namespacePrefixCache = new ConcurrentHashMap<>();

/**
* Cache of namespace prefixes per aggregate path. This cache stores the discovered prefixes
* for each aggregate path to avoid re-walking the same subtrees.
*/
private final ConcurrentHashMap<String, String[]> aggregateNamespaceCache = new ConcurrentHashMap<>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unbounded cache; and with a potentially very large amount of paths this can lead a huge memory consumption.


/**
* config
*/
Expand Down Expand Up @@ -301,6 +319,9 @@ private AggregateManagerImpl(

// setup node types
initNodeTypes();

// pre-populate namespace cache with standard JCR namespaces
initNamespaceCache();
}

public Set<String> getNodeTypes() {
Expand All @@ -324,6 +345,100 @@ public String getNamespaceURI(String prefix) throws RepositoryException {
return session.getNamespaceURI(prefix);
}

/**
* Gets a namespace URI from the cache or from the session if not cached.
* This method caches the prefix-to-URI mapping to avoid repeated JCR lookups.
*
* @param prefix the namespace prefix
* @return the namespace URI
* @throws RepositoryException if an error occurs
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this better than using the JCR namespace registry? Why do we need a cache here?

public String getCachedNamespaceURI(String prefix) throws RepositoryException {
return namespacePrefixCache.computeIfAbsent(prefix, p -> {
try {
return session.getNamespaceURI(p);
} catch (RepositoryException e) {
throw new RuntimeException("Failed to get namespace URI for prefix: " + p, e);
}
});
}

/**
* Adds a namespace prefix to the cache.
*
* @param prefix the namespace prefix to cache
*/
public void cacheNamespacePrefix(String prefix) {
if (prefix != null && !prefix.isEmpty() && !namespacePrefixCache.containsKey(prefix)) {
try {
String uri = session.getNamespaceURI(prefix);
namespacePrefixCache.put(prefix, uri);
} catch (RepositoryException e) {
// Log but don't fail - the prefix might be checked later
log.debug("Could not resolve namespace URI for prefix: {}", prefix, e);
}
}
}

/**
* Gets the cached namespace prefixes.
*
* @return a set of all cached namespace prefixes
*/
public Set<String> getCachedNamespacePrefixes() {
return namespacePrefixCache.keySet();
}

/**
* Gets cached namespace prefixes for a specific aggregate path.
*
* @param path the aggregate path
* @return the cached prefixes, or null if not cached
*/
public String[] getCachedAggregatePrefixes(String path) {
return aggregateNamespaceCache.get(path);
}

/**
* Caches namespace prefixes for a specific aggregate path.
*
* @param path the aggregate path
* @param prefixes the namespace prefixes to cache
*/
public void cacheAggregatePrefixes(String path, String[] prefixes) {
if (path != null && prefixes != null) {
aggregateNamespaceCache.put(path, prefixes);
}
}

/**
* Invalidates the namespace caches. This should be called if namespace
* definitions are added or modified in the repository.
*/
public void invalidateNamespaceCaches() {
log.info(
"Invalidating namespace caches ({} prefix mappings, {} aggregate caches)",
namespacePrefixCache.size(),
aggregateNamespaceCache.size());
namespacePrefixCache.clear();
aggregateNamespaceCache.clear();
// Re-initialize the prefix cache with current repository namespaces
initNamespaceCache();
}

/**
* Invalidates the aggregate namespace cache for a specific path.
* This should be called when content at that path is modified.
*
* @param path the aggregate path to invalidate
*/
public void invalidateAggregatePrefixes(String path) {
if (path != null) {
aggregateNamespaceCache.remove(path);
log.debug("Invalidated namespace cache for path: {}", path);
}
}

public void startTracking(ProgressTrackerListener pTracker) {
tracker = new AggregatorTracker(pTracker);
}
Expand Down Expand Up @@ -426,6 +541,27 @@ private void initNodeTypes() throws RepositoryException {
}
}

/**
* Pre-populates the namespace cache with all namespaces registered in the repository.
* This optimization reduces expensive JCR tree traversals during namespace discovery.
*/
private void initNamespaceCache() {
try {
String[] prefixes = session.getNamespacePrefixes();
for (String prefix : prefixes) {
try {
String uri = session.getNamespaceURI(prefix);
namespacePrefixCache.put(prefix, uri);
} catch (RepositoryException e) {
log.debug("Could not cache namespace prefix '{}': {}", prefix, e.getMessage());
}
}
log.info("Initialized namespace cache with {} prefixes", namespacePrefixCache.size());
} catch (RepositoryException e) {
log.warn("Could not initialize namespace cache", e);
}
}

public Aggregator getAggregator(Node node, String path) throws RepositoryException {
return aggregatorProvider.getAggregator(node, path);
}
Expand Down