[
https://issues.apache.org/jira/browse/AVRO-2051?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16678513#comment-16678513
]
ASF GitHub Bot commented on AVRO-2051:
--------------------------------------
dkulp closed pull request #245: [AVRO-2051] Remove synchronization for
JsonProperties.getJsonProp
URL: https://github.com/apache/avro/pull/245
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
b/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
index 4e18c09b0..368c29e75 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
@@ -17,12 +17,20 @@
*/
package org.apache.avro;
+import java.util.AbstractSet;
import java.util.Collections;
+import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.Map;
+import java.util.Queue;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ConcurrentMap;
+
import java.io.IOException;
+import org.apache.avro.reflect.MapEntry;
import org.apache.avro.util.internal.JacksonUtils;
import org.codehaus.jackson.JsonNode;
import org.codehaus.jackson.JsonGenerator;
@@ -114,7 +122,45 @@ private Null() {}
/** A value representing a JSON <code>null</code>. */
public static final Null NULL_VALUE = new Null();
- Map<String,JsonNode> props = new LinkedHashMap<String,JsonNode>(1);
+ // use a ConcurrentHashMap for speed and thread safety, but keep a Queue of
the entries to maintain order
+ // the queue is always updated after the main map and is thus is potentially
a subset of the map.
+ // By making props private, we can control access and only
implement/override the methods
+ // we need. We don't ever remove anything so we don't need to implement the
clear/remove functionality.
+ // Also, we only ever ADD to the collection, never changing a value, so
putWithAbsent is the
+ // only modifier
+ private ConcurrentMap<String,JsonNode> props = new
ConcurrentHashMap<String,JsonNode>() {
+ private static final long serialVersionUID = 1L;
+ private Queue<MapEntry<String, JsonNode>> propOrder = new
ConcurrentLinkedQueue<MapEntry<String, JsonNode>>();
+ public JsonNode putIfAbsent(String key, JsonNode value) {
+ JsonNode r = super.putIfAbsent(key, value);
+ if (r == null) {
+ propOrder.add(new MapEntry<String, JsonNode>(key, value));
+ }
+ return r;
+ }
+ public Set<Map.Entry<String, JsonNode>> entrySet() {
+ return new AbstractSet<Map.Entry<String, JsonNode>>() {
+ @Override
+ public Iterator<Map.Entry<String, JsonNode>> iterator() {
+ return new Iterator<Map.Entry<String, JsonNode>>() {
+ Iterator<MapEntry<String, JsonNode>> it = propOrder.iterator();
+ @Override
+ public boolean hasNext() {
+ return it.hasNext();
+ }
+ @Override
+ public java.util.Map.Entry<String, JsonNode> next() {
+ return it.next();
+ }
+ };
+ }
+ @Override
+ public int size() {
+ return propOrder.size();
+ }
+ };
+ }
+ };
private Set<String> reserved;
@@ -137,7 +183,7 @@ public String getProp(String name) {
* @deprecated use {@link #getObjectProp(String)}
*/
@Deprecated
- public synchronized JsonNode getJsonProp(String name) {
+ public JsonNode getJsonProp(String name) {
return props.get(name);
}
@@ -145,7 +191,7 @@ public synchronized JsonNode getJsonProp(String name) {
* Returns the value of the named property in this schema.
* Returns <tt>null</tt> if there is no property with that name.
*/
- public synchronized Object getObjectProp(String name) {
+ public Object getObjectProp(String name) {
return JacksonUtils.toObject(props.get(name));
}
@@ -173,23 +219,26 @@ public void addProp(String name, String value) {
* @deprecated use {@link #addProp(String, Object)}
*/
@Deprecated
- public synchronized void addProp(String name, JsonNode value) {
+ public void addProp(String name, JsonNode value) {
if (reserved.contains(name))
throw new AvroRuntimeException("Can't set reserved property: " + name);
if (value == null)
throw new AvroRuntimeException("Can't set a property to null: " + name);
- JsonNode old = props.get(name);
- if (old == null)
- props.put(name, value);
- else if (!old.equals(value))
+ JsonNode old = props.putIfAbsent(name, value);
+ if (old != null && !old.equals(value)) {
throw new AvroRuntimeException("Can't overwrite property: " + name);
+ }
}
-
- public synchronized void addProp(String name, Object value) {
+ public void addProp(String name, Object value) {
addProp(name, JacksonUtils.toJsonNode(value));
}
+ public void putAll(JsonProperties np) {
+ for (Map.Entry<? extends String, ? extends JsonNode> e :
np.props.entrySet())
+ addProp(e.getKey(), e.getValue());
+ }
+
/** Return the defined properties that have string values. */
@Deprecated public Map<String,String> getProps() {
@@ -200,14 +249,6 @@ public synchronized void addProp(String name, Object
value) {
return result;
}
- /** Convert a map of string-valued properties to Json properties. */
- Map<String,JsonNode> jsonProps(Map<String,String> stringProps) {
- Map<String,JsonNode> result = new LinkedHashMap<String,JsonNode>();
- for (Map.Entry<String,String> e : stringProps.entrySet())
- result.put(e.getKey(), TextNode.valueOf(e.getValue()));
- return result;
- }
-
/**
* Return the defined properties as an unmodifieable Map.
* @deprecated use {@link #getObjectProps()}
@@ -230,4 +271,15 @@ void writeProps(JsonGenerator gen) throws IOException {
gen.writeObjectField(e.getKey(), e.getValue());
}
+
+ int propsHashCode() {
+ return props.hashCode();
+ }
+ boolean propsEqual(JsonProperties np) {
+ return props.equals(np.props);
+ }
+ public boolean hasProps() {
+ return !props.isEmpty();
+ }
+
}
diff --git a/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
b/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
index 73e235c89..959122759 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
@@ -147,11 +147,11 @@ public boolean equals(Object o) {
Message that = (Message)o;
return this.name.equals(that.name)
&& this.request.equals(that.request)
- && props.equals(that.props);
+ && propsEqual(that);
}
public int hashCode() {
- return name.hashCode() + request.hashCode() + props.hashCode();
+ return name.hashCode() + request.hashCode() + propsHashCode();
}
public String getDoc() { return doc; }
@@ -298,12 +298,12 @@ public boolean equals(Object o) {
&& this.namespace.equals(that.namespace)
&& this.types.equals(that.types)
&& this.messages.equals(that.messages)
- && this.props.equals(that.props);
+ && this.propsEqual(that);
}
public int hashCode() {
return name.hashCode() + namespace.hashCode()
- + types.hashCode() + messages.hashCode() + props.hashCode();
+ + types.hashCode() + messages.hashCode() + propsHashCode();
}
/** Render this as <a href="http://json.org/";>JSON</a>.*/
diff --git a/lang/java/avro/src/main/java/org/apache/avro/Schema.java
b/lang/java/avro/src/main/java/org/apache/avro/Schema.java
index f01775d64..ea693796d 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/Schema.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/Schema.java
@@ -330,7 +330,7 @@ public String toString(boolean pretty) {
}
void toJson(Names names, JsonGenerator gen) throws IOException {
- if (props.size() == 0) { // no props defined
+ if (!hasProps()) { // no props defined
gen.writeString(getName()); // just write name
} else {
gen.writeStartObject();
@@ -349,7 +349,7 @@ public boolean equals(Object o) {
if (!(o instanceof Schema)) return false;
Schema that = (Schema)o;
if (!(this.type == that.type)) return false;
- return equalCachedHash(that) && props.equals(that.props);
+ return equalCachedHash(that) && propsEqual(that);
}
public final int hashCode() {
if (hashCode == NO_HASHCODE)
@@ -357,7 +357,7 @@ public final int hashCode() {
return hashCode;
}
- int computeHash() { return getType().hashCode() + props.hashCode(); }
+ int computeHash() { return getType().hashCode() + propsHashCode(); }
final boolean equalCachedHash(Schema other) {
return (hashCode == other.hashCode)
@@ -457,7 +457,7 @@ public boolean equals(Object other) {
(schema.equals(that.schema)) &&
defaultValueEquals(that.defaultValue) &&
(order == that.order) &&
- props.equals(that.props);
+ propsEqual(that);
}
public int hashCode() { return name.hashCode() + schema.computeHash(); }
@@ -666,7 +666,7 @@ public boolean equals(Object o) {
RecordSchema that = (RecordSchema)o;
if (!equalCachedHash(that)) return false;
if (!equalNames(that)) return false;
- if (!props.equals(that.props)) return false;
+ if (!propsEqual(that)) return false;
Set seen = SEEN_EQUALS.get();
SeenPair here = new SeenPair(this, o);
if (seen.contains(here)) return true; // prevent stack overflow
@@ -763,7 +763,7 @@ public boolean equals(Object o) {
return equalCachedHash(that)
&& equalNames(that)
&& symbols.equals(that.symbols)
- && props.equals(that.props);
+ && propsEqual(that);
}
@Override int computeHash() { return super.computeHash() +
symbols.hashCode(); }
void toJson(Names names, JsonGenerator gen) throws IOException {
@@ -796,7 +796,7 @@ public boolean equals(Object o) {
ArraySchema that = (ArraySchema)o;
return equalCachedHash(that)
&& elementType.equals(that.elementType)
- && props.equals(that.props);
+ && propsEqual(that);
}
@Override int computeHash() {
return super.computeHash() + elementType.computeHash();
@@ -824,7 +824,7 @@ public boolean equals(Object o) {
MapSchema that = (MapSchema)o;
return equalCachedHash(that)
&& valueType.equals(that.valueType)
- && props.equals(that.props);
+ && propsEqual(that);
}
@Override int computeHash() {
return super.computeHash() + valueType.computeHash();
@@ -865,7 +865,7 @@ public boolean equals(Object o) {
UnionSchema that = (UnionSchema)o;
return equalCachedHash(that)
&& types.equals(that.types)
- && props.equals(that.props);
+ && propsEqual(that);
}
@Override int computeHash() {
int hash = super.computeHash();
@@ -903,7 +903,7 @@ public boolean equals(Object o) {
return equalCachedHash(that)
&& equalNames(that)
&& size == that.size
- && props.equals(that.props);
+ && propsEqual(that);
}
@Override int computeHash() { return super.computeHash() + size; }
void toJson(Names names, JsonGenerator gen) throws IOException {
@@ -1439,7 +1439,7 @@ private static Schema applyAliases(Schema s,
Map<Schema,Schema> seen,
Schema fSchema = applyAliases(f.schema, seen, aliases, fieldAliases);
String fName = getFieldAlias(name, f.name, fieldAliases);
Field newF = new Field(fName, fSchema, f.doc, f.defaultValue, f.order);
- newF.props.putAll(f.props); // copy props
+ newF.putAll(f); // copy props
newFields.add(newF);
}
result.setFields(newFields);
@@ -1472,7 +1472,7 @@ private static Schema applyAliases(Schema s,
Map<Schema,Schema> seen,
break;
}
if (result != s)
- result.props.putAll(s.props); // copy props
+ result.putAll(s); // copy props
return result;
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Thread contention accessing JsonProperties props
> ------------------------------------------------
>
> Key: AVRO-2051
> URL: https://issues.apache.org/jira/browse/AVRO-2051
> Project: Avro
> Issue Type: Bug
> Components: java
> Affects Versions: 1.8.2
> Reporter: Daniel Kulp
> Priority: Major
>
> See
> https://lists.apache.org/thread.html/dd34ab8439137a81a9de29ad4161f37b17638394cea0806765689976@%3Cuser.avro.apache.org%3E
> Basically, the getJsonProp method, being synchronized, is causing thread
> contention issues when trying to share schemas between threads. My
> proposal (pull request forthcoming) is to treat "props" as an immutable map
> and do a copy+add+swap for the addProp method. This will make the addProp
> call slower (particularly for large maps of props), but would make the reads
> significantly faster as no locking will be needed.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
