Skip to content

Commit

Permalink
[GEOS-7953] GSIP 155 - Core improvements for catalogs with many layers
Browse files Browse the repository at this point in the history
  • Loading branch information
aaime committed Jan 28, 2017
1 parent 2673e47 commit 75e4c91
Show file tree
Hide file tree
Showing 23 changed files with 642 additions and 536 deletions.
Expand Up @@ -8,6 +8,7 @@
import static com.google.common.base.Throwables.propagate; import static com.google.common.base.Throwables.propagate;
import static com.google.common.base.Throwables.propagateIfInstanceOf; import static com.google.common.base.Throwables.propagateIfInstanceOf;


import java.io.ByteArrayInputStream;
import java.io.FileNotFoundException; import java.io.FileNotFoundException;
import java.io.IOException; import java.io.IOException;
import java.io.InputStreamReader; import java.io.InputStreamReader;
Expand Down Expand Up @@ -292,7 +293,7 @@ private GeoServerTileLayerInfoImpl depersist(final Resource res) throws IOExcept
LOGGER.fine("Depersisting GeoServerTileLayerInfo from " + res.path()); LOGGER.fine("Depersisting GeoServerTileLayerInfo from " + res.path());
} }
GeoServerTileLayerInfoImpl info; GeoServerTileLayerInfoImpl info;
try(Reader reader = new InputStreamReader(res.in(), "UTF-8")) { try(Reader reader = new InputStreamReader(new ByteArrayInputStream(res.getContents()), "UTF-8")) {
info = (GeoServerTileLayerInfoImpl) serializer.fromXML(reader); info = (GeoServerTileLayerInfoImpl) serializer.fromXML(reader);
} }


Expand Down
Expand Up @@ -267,11 +267,11 @@ public void visit(LayerGroupInfo layerGroupToRemove) {
LayerGroupInfo group = it.next(); LayerGroupInfo group = it.next();


// parallel remove of layer and styles // parallel remove of layer and styles
int index = group.getLayers().indexOf(layerGroupToRemove); int index = getLayerGroupIndex(layerGroupToRemove, group);
while (index != -1) { while (index != -1) {
group.getLayers().remove(index); group.getLayers().remove(index);
group.getStyles().remove(index); group.getStyles().remove(index);
index = group.getLayers().indexOf(layerGroupToRemove); index = getLayerGroupIndex(layerGroupToRemove, group);
} }
if (group.getLayers().size() == 0) { if (group.getLayers().size() == 0) {
// if group is empty, delete it // if group is empty, delete it
Expand All @@ -286,6 +286,28 @@ public void visit(LayerGroupInfo layerGroupToRemove) {
catalog.remove(layerGroupToRemove); catalog.remove(layerGroupToRemove);
} }


/**
* Between modification proxies and security buffering the list of layers of a group it's just
* safer and more predictable to use a id comparison instead of a equals that accounts for
* each and every field
*
* @param layerGroup
* @param container
* @return
*/
private int getLayerGroupIndex(LayerGroupInfo layerGroup, LayerGroupInfo container) {
int idx = 0;
final String id = layerGroup.getId();
for (PublishedInfo pi : container.getLayers()) {
if(pi instanceof LayerGroupInfo && id.equals(pi.getId())) {
return idx;
}
idx++;
}

return -1;
}

public void visit(WMSLayerInfo wmsLayer) { public void visit(WMSLayerInfo wmsLayer) {
catalog.remove(wmsLayer); catalog.remove(wmsLayer);


Expand Down
Expand Up @@ -1069,9 +1069,10 @@ public void add(NamespaceInfo namespace) {


NamespaceInfo added; NamespaceInfo added;
synchronized (facade) { synchronized (facade) {
added = facade.add(resolve(namespace)); final NamespaceInfo resolved = resolve(namespace);
added = facade.add(resolved);
if ( getDefaultNamespace() == null ) { if ( getDefaultNamespace() == null ) {
setDefaultNamespace(namespace); setDefaultNamespace(resolved);
} }
} }


Expand Down Expand Up @@ -1386,10 +1387,8 @@ public ValidationResult validate( StyleInfo style, boolean isNew ) {


public void remove(StyleInfo style) { public void remove(StyleInfo style) {
//ensure no references to the style //ensure no references to the style
for ( LayerInfo l : facade.getLayers() ) { for ( LayerInfo l : facade.getLayers(style) ) {
if ( style.equals( l.getDefaultStyle() ) || l.getStyles().contains( style )) { throw new IllegalArgumentException( "Unable to delete style referenced by '"+ l.getName()+"'");
throw new IllegalArgumentException( "Unable to delete style referenced by '"+ l.getName()+"'");
}
} }


for ( LayerGroupInfo lg : facade.getLayerGroups() ) { for ( LayerGroupInfo lg : facade.getLayerGroups() ) {
Expand Down
@@ -0,0 +1,213 @@
/* (c) 2017 Open Source Geospatial Foundation - all rights reserved
* This code is licensed under the GPL 2.0 license, available at the root
* application directory.
*/
package org.geoserver.catalog.impl;

import java.lang.reflect.Proxy;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentSkipListMap;
import java.util.function.Function;
import java.util.function.Predicate;

import org.geoserver.catalog.CatalogInfo;
import org.opengis.feature.type.Name;

/**
* A support index for {@link DefaultCatalogFacade}, can perform fast lookups of {@link CatalogInfo} objects
* by id or by "name", where the name is defined by a a user provided mapping function.
*
* The lookups by predicate have been tested and optimized for performance, in particular
* the current for loops turned out to be significantly faster than building and returning streams
*
* @param <T>
*/
class CatalogInfoLookup<T extends CatalogInfo> {
ConcurrentHashMap<Class<T>, Map<String, T>> idMultiMap = new ConcurrentHashMap<>();
ConcurrentHashMap<Class<T>, Map<Name, T>> nameMultiMap = new ConcurrentHashMap<>();
Function<T, Name> nameMapper;
static final Predicate TRUE = x -> true;

public CatalogInfoLookup(Function<T, Name> nameMapper) {
super();
this.nameMapper = nameMapper;
}

<K> Map<K, T> getMapForValue(ConcurrentHashMap<Class<T>, Map<K, T>> maps, T value) {
Class<T> vc;
if(Proxy.isProxyClass(value.getClass())) {
ModificationProxy h = (ModificationProxy) Proxy.getInvocationHandler(value);
Object po = (T) h.getProxyObject();
vc = (Class<T>) po.getClass();
} else {
vc = (Class<T>) value.getClass();
}


return getMapForValue(maps, vc);
}

protected <K> Map<K, T> getMapForValue(ConcurrentHashMap<Class<T>, Map<K, T>> maps, Class vc) {
Map<K, T> vcMap = maps.get(vc);
if(vcMap == null) {
vcMap = maps.computeIfAbsent(vc, k -> new ConcurrentSkipListMap<K, T>());
}
return vcMap;
}

public T add(T value) {
if(Proxy.isProxyClass(value.getClass())) {
ModificationProxy h = (ModificationProxy) Proxy.getInvocationHandler(value);
value = (T) h.getProxyObject();
}
Map<Name, T> nameMap = getMapForValue(nameMultiMap, value);
Name name = nameMapper.apply(value);
nameMap.put(name, value);
Map<String, T> idMap = getMapForValue(idMultiMap, value);
return idMap.put(value.getId(), value);
}

public Collection<T> values() {
List<T> result = new ArrayList<>();
for (Map<String, T> v : idMultiMap.values()) {
result.addAll(v.values());
}

return result;
}

public T remove(T value) {
Name name = nameMapper.apply(value);
Map<Name, T> nameMap = getMapForValue(nameMultiMap, value);
nameMap.remove(name);
Map<String, T> idMap = getMapForValue(idMultiMap, value);
return idMap.remove(value.getId());
}

/**
* Updates the value in the name map. The new value must be a ModificationProxy
*/
public void update(T proxiedValue) {
ModificationProxy h = (ModificationProxy) Proxy.getInvocationHandler(proxiedValue);
T actualValue = (T) h.getProxyObject();

Name oldName = nameMapper.apply(actualValue);
Name newName = nameMapper.apply(proxiedValue);
if(!oldName.equals(newName)) {
Map<Name, T> nameMap = getMapForValue(nameMultiMap, actualValue);
nameMap.remove(oldName);
nameMap.put(newName, actualValue);
}
}


public void clear() {
idMultiMap.clear();
nameMultiMap.clear();
}

/**
* Looks up objects by class and matching predicate.
*
* This method is significantly faster than creating a stream and the applying the predicate
* on it. Just using this approach instead of the stream makes the overall startup of GeoServer with 20k
* layers go down from 50s to 44s (which is a lot, considering there is a lot of other things going on)
* @param clazz
* @param predicate
* @return
*/
<U extends CatalogInfo> List<U> list(Class<U> clazz, Predicate<U> predicate) {
ArrayList<U> result = new ArrayList<U>();
for (Class<T> key : nameMultiMap.keySet()) {
if (clazz.isAssignableFrom(key)) {
Map<Name, T> valueMap = nameMultiMap.get(key);
if (valueMap != null) {
for (T v : valueMap.values()) {
final U u = (U) v;
if (predicate == TRUE || predicate.test(u)) {
result.add(u);
}
}
}
}
}

return result;
}

/**
* Looks up a CatalogInfo by class and identifier
* @param id
* @param clazz
* @return
*/
public <U extends CatalogInfo> U findById(String id, Class<U> clazz) {
for (Class<T> key : idMultiMap.keySet()) {
if (clazz.isAssignableFrom(key)) {
Map<String, T> valueMap = idMultiMap.get(key);
if(valueMap != null) {
T t = valueMap.get(id);
if(t != null) {
return (U) t;
}
}
}
}

return null;
}

/**
* Looks up a CatalogInfo by class and name
* @param clazz
* @param id
* @return
*/
public <U extends CatalogInfo> U findByName(Name name, Class<U> clazz) {
for (Class<T> key : nameMultiMap.keySet()) {
if (clazz.isAssignableFrom(key)) {
Map<Name, T> valueMap = nameMultiMap.get(key);
if(valueMap != null) {
T t = valueMap.get(name);
if(t != null) {
return (U) t;
}
}
}
}

return null;
}

/**
* Looks up objects by class and matching predicate.
*
* This method is significantly faster than creating a stream and the applying the predicate
* on it. Just using this approach instead of the stream makes the overall startup of GeoServer with 20k
* layers go down from 50s to 44s (which is a lot, considering there is a lot of other things going on)
* @param clazz
* @param predicate
* @return
*/
<U extends CatalogInfo> U findFirst(Class<U> clazz, Predicate<U> predicate) {
for (Class<T> key : nameMultiMap.keySet()) {
if (clazz.isAssignableFrom(key)) {
Map<Name, T> valueMap = nameMultiMap.get(key);
if (valueMap != null) {
for (T v : valueMap.values()) {
final U u = (U) v;
if (predicate == TRUE || predicate.test(u)) {
return u;
}
}
}
}
}

return null;
}
}

2 comments on commit 75e4c91

@guischulz
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest to also catch InvalidPathException in FileSystemResource.getType().
Otherwise the test testSpecialCharacterNames will fail on windows.

testSpecialCharacterNames(org.geoserver.rest.resources.ResourceTest) Time elapsed: 16 sec <<< ERROR! java.nio.file.InvalidPathException: Illegal char <?> at index 42: .\target\default4639919193507169470data\po?zie at sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182) at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153) at sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77) at sun.nio.fs.WindowsPath.parse(WindowsPath.java:94) at sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:255) at java.io.File.toPath(File.java:2234) at org.geoserver.platform.resource.FileSystemResourceStore$FileSystemResource.getType(FileSystemResourceStore.java:431) at org.geoserver.rest.resources.ResourceTest.testSpecialCharacterNames(ResourceTest.java:165)

@aaime
Copy link
Member Author

@aaime aaime commented on 75e4c91 Mar 11, 2017

Choose a reason for hiding this comment

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

Hi, can you make a pull request with that change? None of the core developers uses windows, we cannot see what other changes might be needed for the test to work

Please sign in to comment.