Skip to content

Commit

Permalink
Ignore no-op load/save/delete actions properly
Browse files Browse the repository at this point in the history
* the goal is to avoid the small, but potentially frequently executed calls to save CPU
* removal of proxy creation in favor of forwarding list/map
  • Loading branch information
nbali committed Oct 25, 2015
1 parent 2edd76e commit edb04f2
Show file tree
Hide file tree
Showing 14 changed files with 244 additions and 37 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ objectify.iml
.svn
.gwt
.gradle
.factorypath
7 changes: 7 additions & 0 deletions src/main/java/com/googlecode/objectify/impl/DeleterImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.googlecode.objectify.Result;
import com.googlecode.objectify.cmd.DeleteType;
import com.googlecode.objectify.cmd.Deleter;
import com.googlecode.objectify.util.ResultNow;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -58,6 +59,9 @@ public Result<Void> keys(Iterable<? extends Key<?>> keys) {
for (Key<?> key: keys)
rawKeys.add(key.getRaw());

if (rawKeys.isEmpty())
return new ResultNow<Void>(null);

return ofy.createWriteEngine().delete(rawKeys);
}

Expand All @@ -78,6 +82,9 @@ public Result<Void> entities(Iterable<?> entities) {
for (Object obj: entities)
keys.add(ofy.factory().keys().anythingToRawKey(obj));

if (keys.isEmpty())
return new ResultNow<Void>(null);

return ofy.createWriteEngine().delete(keys);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public Result<Map<Key<?>, Object>> translate(final Result<Map<com.google.appengi

/** */
@Override
public Map<Key<?>, Object> nowUncached() {
protected Map<Key<?>, Object> nowUncached() {
Map<Key<?>, Object> result = new HashMap<>(raw.now().size() * 2);

ctx = new LoadContext(LoadEngine.this);
Expand Down
14 changes: 9 additions & 5 deletions src/main/java/com/googlecode/objectify/impl/LoadTypeImpl.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.googlecode.objectify.impl;

import com.google.appengine.api.datastore.Query.Filter;
import com.google.common.collect.Maps;
import com.googlecode.objectify.Key;
import com.googlecode.objectify.LoadResult;
import com.googlecode.objectify.cmd.LoadIds;
Expand Down Expand Up @@ -85,15 +86,15 @@ public Query<T> order(String condition) {
*/
@Override
public LoadResult<T> id(long id) {
return loader.key(this.<T>makeKey(id));
return loader.key(this.makeKey(id));
}

/* (non-Javadoc)
* @see com.googlecode.objectify.cmd.LoadIds#id(java.lang.String)
*/
@Override
public LoadResult<T> id(String id) {
return loader.key(this.<T>makeKey(id));
return loader.key(this.makeKey(id));
}

/* (non-Javadoc)
Expand All @@ -120,11 +121,14 @@ public <S> Map<S, T> ids(Iterable<S> ids) {

final Map<Key<T>, S> keymap = new LinkedHashMap<>();
for (S id: ids)
keymap.put(this.<T>makeKey(id), id);
keymap.put(this.makeKey(id), id);

if (keymap.isEmpty())
return Maps.newLinkedHashMap();

final Map<Key<T>, T> loaded = loader.keys(keymap.keySet());

return ResultProxy.create(Map.class, new ResultCache<Map<S, T>>() {
return ResultProxy.createMap(new ResultCache<Map<S, T>>() {
@Override
protected Map<S, T> nowUncached() {
Map<S, T> proper = new LinkedHashMap<>(loaded.size() * 2);
Expand All @@ -140,7 +144,7 @@ protected Map<S, T> nowUncached() {
/**
* Make a key for the given id
*/
private <T> Key<T> makeKey(Object id) {
private Key<T> makeKey(Object id) {
return DatastoreUtils.createKey(parent, kind, id);
}

Expand Down
7 changes: 5 additions & 2 deletions src/main/java/com/googlecode/objectify/impl/LoaderImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ public <E> Map<Key<E>, E> values(Iterable<?> values) {
for (Object keyish: values)
keys.add((Key<E>)ofy.factory().keys().anythingToKey(keyish));

if (keys.isEmpty())
return Maps.newLinkedHashMap();

LoadEngine engine = createLoadEngine();

final Map<Key<E>, Result<E>> results = new LinkedHashMap<>();
Expand All @@ -194,9 +197,9 @@ public <E> Map<Key<E>, E> values(Iterable<?> values) {
// Now asynchronously translate into a normal-looking map. We must be careful to exclude results with
// null (missing) values because that is the contract established by DatastoreService.get().
// We use the ResultProxy and create a new map because the performance of filtered views is questionable.
return ResultProxy.create(Map.class, new ResultCache<Map<Key<E>, E>>() {
return ResultProxy.createMap(new ResultCache<Map<Key<E>, E>>() {
@Override
public Map<Key<E>, E> nowUncached() {
protected Map<Key<E>, E> nowUncached() {
return
Maps.newLinkedHashMap(
Maps.filterValues(
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/googlecode/objectify/impl/QueryImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ public QueryResultIterator<T> iterator() {
*/
@Override
public List<T> list() {
return ResultProxy.create(List.class, new MakeListResult<>(this.chunk(Integer.MAX_VALUE).iterable()));
return ResultProxy.createList(new MakeListResult<>(this.chunk(Integer.MAX_VALUE).iterable()));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public QueryResultIterable<Key<T>> iterable() {

@Override
public List<Key<T>> list() {
return ResultProxy.create(List.class, new MakeListResult<>(impl.chunk(Integer.MAX_VALUE).keysIterable()));
return ResultProxy.createList(new MakeListResult<>(impl.chunk(Integer.MAX_VALUE).keysIterable()));
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/googlecode/objectify/impl/Round.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public <T> Result<T> get(final Key<T> key) {
Result<T> result = new ResultCache<T>() {
@Override
@SuppressWarnings("unchecked")
public T nowUncached() {
protected T nowUncached() {
// Because clients could conceivably get() in the middle of our operations (see LoadCollectionRefsTest.specialListWorks()),
// we need to check for early execution. This will perform poorly, but at least it will work.
//assert Round.this.isExecuted();
Expand Down Expand Up @@ -167,9 +167,9 @@ private Result<Map<com.google.appengine.api.datastore.Key, Entity>> fetchPending
} else {
final Result<Map<com.google.appengine.api.datastore.Key, Entity>> fetched = loadEngine.fetch(fetch);

return new Result<Map<com.google.appengine.api.datastore.Key, Entity>>() {
return new ResultCache<Map<com.google.appengine.api.datastore.Key, Entity>>() {
@Override
public Map<com.google.appengine.api.datastore.Key, Entity> now() {
protected Map<com.google.appengine.api.datastore.Key, Entity> nowUncached() {
combined.putAll(fetched.now());
return combined;
}
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/googlecode/objectify/impl/SaverImpl.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package com.googlecode.objectify.impl;

import com.google.appengine.api.datastore.Entity;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.googlecode.objectify.Key;
import com.googlecode.objectify.Result;
import com.googlecode.objectify.cmd.Saver;
import com.googlecode.objectify.impl.translate.SaveContext;
import com.googlecode.objectify.util.ResultNow;
import com.googlecode.objectify.util.ResultWrapper;

import java.util.Arrays;
Expand Down Expand Up @@ -57,6 +60,9 @@ public <E> Result<Map<Key<E>, E>> entities(E... entities) {
*/
@Override
public <E> Result<Map<Key<E>, E>> entities(final Iterable<E> entities) {
if (Iterables.isEmpty(entities))
return new ResultNow<Map<Key<E>, E>>(Maps.<Key<E>, E> newLinkedHashMap());

return ofy.createWriteEngine().<E>save(entities);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ abstract public class ResultCache<T> implements Result<T>, Serializable
protected abstract T nowUncached();

/** */
protected boolean isExecuted() {
protected final boolean isExecuted() {
return cached;
}

Expand Down
60 changes: 38 additions & 22 deletions src/main/java/com/googlecode/objectify/util/ResultProxy.java
Original file line number Diff line number Diff line change
@@ -1,41 +1,57 @@
package com.googlecode.objectify.util;

import com.google.common.collect.ForwardingList;
import com.google.common.collect.ForwardingMap;
import com.googlecode.objectify.Result;

import java.io.ObjectStreamException;
import java.io.Serializable;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.List;
import java.util.Map;

import lombok.RequiredArgsConstructor;

/**
* A dynamic proxy that wraps a Result<?> value. For example, if you had a Result<List<String>>, the
* A utility class that wraps a Result<?> value. For example, if you had a Result<List<String>>, the
* proxy would implement List<String> and call through to the inner object.
*/
public class ResultProxy<T> implements InvocationHandler, Serializable
public class ResultProxy<T>
{
/**
* Create a ResultProxy for the given interface.
*/
@SuppressWarnings("unchecked")
public static <S> S create(Class<? super S> interf, Result<S> result) {
return (S)Proxy.newProxyInstance(result.getClass().getClassLoader(), new Class[] { interf }, new ResultProxy<>(result));
public static <K, V> Map<K, V> createMap(final Result<Map<K, V>> result) {
return new ResultProxyMap<K, V>(result);
}

Result<T> result;
public static <E> List<E> createList(final Result<List<E>> result) {
return new ResultProxyList<E>(result);
}

@RequiredArgsConstructor
private static class ResultProxyList<E> extends ForwardingList<E> implements Serializable {

private final Result<List<E>> result;

@Override
protected List<E> delegate() {
return result.now();
}

private ResultProxy(Result<T> result) {
this.result = result;
private Object writeReplace() throws ObjectStreamException {
return delegate();
}
}

@Override
public Object invoke(Object obj, Method meth, Object[] params) throws Throwable {
return meth.invoke(result.now(), params);
@RequiredArgsConstructor
private static class ResultProxyMap<K, V> extends ForwardingMap<K, V> implements Serializable {

private final Result<Map<K, V>> result;

@Override
protected Map<K, V> delegate() {
return result.now();
}

private Object writeReplace() throws ObjectStreamException {
return delegate();
}
}

private Object writeReplace() throws ObjectStreamException {
return new NowProxy<>(result.now());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public ResultTranslator(F from) {
protected abstract T translate(F from);

@Override
public T nowUncached() {
protected final T nowUncached() {
return translate(from);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package com.googlecode.objectify.test;

import static com.googlecode.objectify.ObjectifyService.factory;
import static com.googlecode.objectify.ObjectifyService.ofy;

import java.util.Arrays;

import org.mockito.Mockito;
import org.testng.annotations.Test;

import com.google.appengine.api.datastore.DatastoreServiceConfig;
import com.google.common.collect.Lists;
import com.googlecode.objectify.Key;
import com.googlecode.objectify.test.entity.Trivial;
import com.googlecode.objectify.test.util.TestBase;
import com.googlecode.objectify.test.util.TestObjectifyFactory;

public class IgnoreEmptyOperationTests extends TestBase {

@Override
protected void setUpObjectifyFactory(TestObjectifyFactory factory) {
factory.register(Trivial.class);
super.setUpObjectifyFactory(Mockito.spy(factory));
Mockito.verify(factory(), Mockito.times(1)).begin();
}

// engines are responsible for datastore actions, and because all of them requires an AsyncDatastoreService
// it seems like a good idea to verify if it has been created or not
private void verifyNoRemoteCall() {
Mockito.verify(factory(), Mockito.never())
.createAsyncDatastoreService(Mockito.any(DatastoreServiceConfig.class), Mockito.anyBoolean());
Mockito.verifyNoMoreInteractions(factory());
}

private void verifyThatRemoteCallWasExecuted(int times) {
Mockito.verify(factory(), Mockito.times(times))
.createAsyncDatastoreService(Mockito.any(DatastoreServiceConfig.class), Mockito.anyBoolean());
}

@Test
public void testEmptySave() {
ofy().save().entities(Lists.newArrayList()).now().size();

verifyNoRemoteCall();
}

@Test
public void testNotEmptySave() {
ofy().save().entities(Arrays.asList(new Trivial())).now().size();

verifyThatRemoteCallWasExecuted(1);
}

@Test
public void testEmptyDelete() {
ofy().delete().entities(Lists.newArrayList()).now();
ofy().delete().keys(Lists.<Key<Trivial>> newArrayList()).now();
ofy().delete().type(Trivial.class).ids(Lists.newArrayList()).now();

verifyNoRemoteCall();
}

@Test
public void testNotEmptyDelete() {
final Trivial trivial = new Trivial(1L, "", 1L);
ofy().delete().entities(Arrays.asList(trivial)).now();
ofy().delete().keys(Arrays.asList(Key.create(trivial))).now();
ofy().delete().type(Trivial.class).ids(Arrays.asList(trivial.getId())).now();

verifyThatRemoteCallWasExecuted(3);
}

@Test
public void testEmptyLoad() {
ofy().load().entities(Lists.newArrayList()).size();
ofy().load().keys(Lists.<Key<Trivial>> newArrayList()).size();
ofy().load().type(Trivial.class).ids(Lists.newArrayList()).size();

verifyNoRemoteCall();
}

@Test
public void testNotEmptyLoad() {
final Trivial trivial = new Trivial(1L, "", 1L);
ofy().load().entities(Arrays.asList(trivial)).size();
ofy().load().keys(Arrays.asList(Key.<Trivial> create(trivial))).size();
ofy().load().type(Trivial.class).ids(Arrays.asList(trivial.getId())).size();

verifyThatRemoteCallWasExecuted(3);
}
}
Loading

0 comments on commit edb04f2

Please sign in to comment.