Skip to content

Remove synchronized methods in favour of ConcurrentHashMap using #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dmitry-grytsovets wants to merge 6 commits into from
Closed

Remove synchronized methods in favour of ConcurrentHashMap using #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8909b2f
remove synchronized methods in favour of ConcurrentHashMap using
Apr 24, 2014
240df62
Revert "remove synchronized methods in favour of ConcurrentHashMap us…
Apr 29, 2014
8cda7d1
Remove synchronized only from lookup method
Apr 29, 2014
2db2f0d
Remove synchronized only from lookup method
Apr 30, 2014
99d9a3f
Remove synchronized only from lookup method
Apr 30, 2014
5db19c1
Change wait notify to countdown latch
Apr 30, 2014
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
16 changes: 16 additions & 0 deletions src/main/java/org/msgpack/template/TemplateReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import java.io.IOException;
import java.lang.reflect.Type;
import java.util.concurrent.CountDownLatch;

import org.msgpack.MessageTypeException;
import org.msgpack.packer.Packer;
Expand All @@ -32,6 +33,9 @@ public class TemplateReference<T> extends AbstractTemplate<T> {

private Template<T> actualTemplate;

private final CountDownLatch ready = new CountDownLatch(1);


public TemplateReference(TemplateRegistry registry, Type targetType) {
this.registry = registry;
this.targetType = targetType;
Expand All @@ -41,13 +45,25 @@ public TemplateReference(TemplateRegistry registry, Type targetType) {
private void validateActualTemplate() {
if (actualTemplate == null) {
actualTemplate = (Template<T>) registry.cache.get(targetType);
if (actualTemplate == this) {
try {
ready.await();
actualTemplate = (Template<T>) registry.cache.get(targetType);
} catch (InterruptedException ignored) {

}
}
if (actualTemplate == null) {
throw new MessageTypeException(
"Actual template have not been created");
}
}
}

public void setReady() {
ready.countDown();
}

@Override
public void write(Packer pk, T v, boolean required) throws IOException {
validateActualTemplate();
Expand Down
19 changes: 11 additions & 8 deletions src/main/java/org/msgpack/template/TemplateRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.nio.ByteBuffer;
import java.util.concurrent.ConcurrentHashMap;

import org.msgpack.MessagePackable;
import org.msgpack.MessageTypeException;
Expand Down Expand Up @@ -76,8 +77,8 @@ public class TemplateRegistry {
private TemplateRegistry() {
parent = null;
chain = createTemplateBuilderChain();
genericCache = new HashMap<Type, GenericTemplate>();
cache = new HashMap<Type, Template<Type>>();
genericCache = new ConcurrentHashMap<Type, GenericTemplate>();
cache = new ConcurrentHashMap<Type, Template<Type>>();
registerTemplates();
cache = Collections.unmodifiableMap(cache);
}
Expand Down Expand Up @@ -196,7 +197,7 @@ public synchronized void unregister() {
cache.clear();
}

public synchronized Template lookup(Type targetType) {
public Template lookup(Type targetType) {
Copy link
Copy Markdown
Member

@komamitsu komamitsu Apr 29, 2014

Choose a reason for hiding this comment

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

Without synchronized keyword, a thread can call register() or unregister() while another thread is executing this method. Did you consider those cases already?

As you know, this method calls register() and cache.get() at some points. So we need to take account of such as the case that thread#A calls unregister() while thread#B is executing in the middle of lookupInterfaceTypes().

Copy link
Copy Markdown
Author

@dmitry-grytsovets dmitry-grytsovets Apr 29, 2014

Choose a reason for hiding this comment

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

I also changed HashMap to ConcurrentHashMap which is fully thread safe. So you can remove all synchronized methods over the maps if you need :) You can look into the ConcurrentHashMap source code where you will find locked write and optimistic read with locked fallback :)

Template tmpl;

if (targetType instanceof ParameterizedType) {
Expand Down Expand Up @@ -550,12 +551,11 @@ private synchronized Template buildAndRegister(TemplateBuilder builder,
final FieldList flist) {
Template newTmpl = null;
Template oldTmpl = null;
TemplateReference ref = null;
try {
if (cache.containsKey(targetClass)) {
oldTmpl = cache.get(targetClass);
}
newTmpl = new TemplateReference(this, targetClass);
cache.put(targetClass, newTmpl);
oldTmpl = cache.get(targetClass);
ref = new TemplateReference(this, targetClass);
cache.put(targetClass, ref);
Copy link
Copy Markdown
Author

@dmitry-grytsovets dmitry-grytsovets Apr 30, 2014

Choose a reason for hiding this comment

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

Move put to the end of template build cause without synchronization other threads can see not initialized version

if (builder == null) {
builder = chain.select(targetClass, hasAnnotation);
}
Expand All @@ -578,6 +578,9 @@ private synchronized Template buildAndRegister(TemplateBuilder builder,
if (newTmpl != null) {
cache.put(targetClass, newTmpl);
}
if(ref != null) {
ref.setReady();
}
}
}

Expand Down