JsonProvider should not create a new ServiceLoader for each provider() invocation

[glassfishrobot]

Affected versions (it's strange, but javax.json.spi.JsonProvider is located both in javax.json:javax.json-api and org.glassfish:javax.json):
javax.json:javax.json-api:1.0 and org.glassfish:javax.json:1.0.4

JsonProvider.provide() is called from Json.createObjectBuilder() and does the following:

public static JsonProvider provider() {
	ServiceLoader<JsonProvider> loader = ServiceLoader.load(JsonProvider.class);
	Iterator<JsonProvider> it = loader.iterator();
	...
}

As you can see it creates a new instance of ServiceLoader (via load(Class) method) each time the method provide() is invoked. Because of this cache of providers maintained by ServiceLoader is not used and loader.iterator() performs heavy search again and again...

The trace looks like

at java.util.zip.ZipFile.getEntry(Native Method)
	at java.util.zip.ZipFile.getEntry(ZipFile.java:311)
	- locked <0x00000000f801b3b8> (a java.util.jar.JarFile)
	at java.util.jar.JarFile.getEntry(JarFile.java:240)
	at java.util.jar.JarFile.getJarEntry(JarFile.java:223)
	at sun.misc.URLClassPath$JarLoader.getResource(URLClassPath.java:841)
	at sun.misc.URLClassPath$JarLoader.findResource(URLClassPath.java:819)
	at sun.misc.URLClassPath$1.next(URLClassPath.java:226)
	at sun.misc.URLClassPath$1.hasMoreElements(URLClassPath.java:236)
	at java.net.URLClassLoader$3$1.run(URLClassLoader.java:589)
	at java.net.URLClassLoader$3$1.run(URLClassLoader.java:587)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader$3.next(URLClassLoader.java:586)
	at java.net.URLClassLoader$3.hasMoreElements(URLClassLoader.java:611)
	at sun.misc.CompoundEnumeration.next(CompoundEnumeration.java:45)
	at sun.misc.CompoundEnumeration.hasMoreElements(CompoundEnumeration.java:54)
	at java.util.ServiceLoader$LazyIterator.hasNextService(ServiceLoader.java:354)
	at java.util.ServiceLoader$LazyIterator.hasNext(ServiceLoader.java:393)
	at java.util.ServiceLoader$1.hasNext(ServiceLoader.java:474)
	at javax.json.spi.JsonProvider.provider(JsonProvider.java:89)
	at javax.json.Json.createObjectBuilder(Json.java:266)

In order to solve this performance problem one should create instance of ServiceLoader only once and reuse it in the JsonProvider.provide() method.

Environment

Oracle Java SE Runtime Environment (build 1.8.0-b132)

Affected Versions

[1.0.4]

[glassfishrobot]

@glassfishrobot Commented
Reported by stIncMale

[glassfishrobot]

@glassfishrobot Commented
kchung said:
Thanks for reporting. Fixed in the next branch

--- a/api/src/main/java/javax/json/spi/JsonProvider.java
+++ b/api/src/main/java/javax/json/spi/JsonProvider.java
@@ -71,6 +71,7 @@ public abstract class JsonProvider {
      */
     private static final String DEFAULT_PROVIDER
             = "org.glassfish.json.JsonProviderImpl";
+    private static ServiceLoader<JsonProvider> loader = ServiceLoader.load(JsonProvider.class);

     protected JsonProvider() {
     }
@@ -85,7 +86,6 @@ public abstract class JsonProvider {
      * @return a JSON provider
      */
     public static JsonProvider provider() {
-        ServiceLoader<JsonProvider> loader = ServiceLoader.load(JsonProvider.class);
         Iterator<JsonProvider> it = loader.iterator();
         if (it.hasNext()) {
             return it.next();

[glassfishrobot]

@glassfishrobot Commented
stincmale said:
No no no You can't just use static ServiceLoader loader field because ServiceLoader is not thread-safe (see documentation: Instances of this class are not safe for use by multiple concurrent threads) So I suppose you should use a ThreadLocal reference to ServiceLoader.

There is also one more consideration. JsonProvider is thread-safe according to its documentation so there is no need to create a new instance of DEFAULT_PROVIDER each time ServiceLoader didn't found any provider: create instance of DEFAULT_PROVIDER once and reuse it. I.e. the following code in the method JsonProvider.provide() should also be refactored:

Class<?> clazz = Class.forName(DEFAULT_PROVIDER);
return (JsonProvider)clazz.newInstance();

P.S unfortunately there is no way to reopen the ticket, but I hope you'll react to this comment.

[glassfishrobot]

@glassfishrobot Commented
kchung said:
Good catch! How about:

--- a/api/src/main/java/javax/json/spi/JsonProvider.java
+++ b/api/src/main/java/javax/json/spi/JsonProvider.java
@@ -71,7 +71,14 @@ public abstract class JsonProvider {
      */
     private static final String DEFAULT_PROVIDER
             = "org.glassfish.json.JsonProviderImpl";
-    private static ServiceLoader<JsonProvider> loader = ServiceLoader.load(JsonProvider.class);
+    private static JsonProvider defaultJsonProvider;
+    private static final ThreadLocal<ServiceLoader<JsonProvider>> threadLoader =
+        new ThreadLocal<ServiceLoader<JsonProvider>>() {
+            @Override
+            protected ServiceLoader<JsonProvider> initialValue() {
+return ServiceLoader.load(JsonProvider.class);
+            }
+        };

     protected JsonProvider() {
     }
@@ -86,14 +93,19 @@ public abstract class JsonProvider {
      * @return a JSON provider
      */
     public static JsonProvider provider() {
-        Iterator<JsonProvider> it = loader.iterator();
+        Iterator<JsonProvider> it = threadLoader.get().iterator();
         if (it.hasNext()) {
             return it.next();
         }

+        if (defaultJsonProvider != null) {
+            return defaultJsonProvider;
+        }
+
         try {
             Class<?> clazz = Class.forName(DEFAULT_PROVIDER);
-            return (JsonProvider)clazz.newInstance();
+            defaultJsonProvider = (JsonProvider)clazz.newInstance();
+            return defaultJsonProvider;
         } catch (ClassNotFoundException x) {
             throw new JsonException(
     "Provider " + DEFAULT_PROVIDER + " not found", x);

[glassfishrobot]

@glassfishrobot Commented
stincmale said:
Unfortunately this code isn't correctly synchronized according to JLS. I see you want to initialize defaultJsonProvider only if it's required (lazy initialization),but you must do it right. According to your notations and the original code, the following code should be correct (note that defaultJsonProvider is volatile)

private static final String DEFAULT_PROVIDER
		= "org.glassfish.json.JsonProviderImpl";
private static volatile JsonProvider defaultJsonProvider;
private static final Object mutex = new Object();
private static final ThreadLocal<ServiceLoader<JsonProvider>> threadLoader
		= new ThreadLocal<ServiceLoader<JsonProvider>>() {
			@Override
			protected ServiceLoader<JsonProvider> initialValue() {
				return ServiceLoader.load(JsonProvider.class);
			}
		};

public static JsonProvider provider() {
	Iterator<JsonProvider> it = threadLoader.get().iterator();
	if (it.hasNext()) {
		return it.next();
	}
	return getDefaultProvider();
}

private static JsonProvider getDefaultProvider() {
	JsonProvider result = defaultJsonProvider;//use local variable in order to reduce number of volatile reads 	if (result == null) {
		synchronized (mutex) {
			result = defaultJsonProvider;
			if (result == null) {
				result = createDefaultProvider();
				defaultJsonProvider = result;
			}
		}
	}
	return result;
}

private static JsonProvider createDefaultProvider() {
	try {
		Class<?> clazz = Class.forName(DEFAULT_PROVIDER);
		return (JsonProvider)clazz.newInstance();
	} catch (ClassNotFoundException x) {
		throw new JsonException(
				"Provider " + DEFAULT_PROVIDER + " not found", x);
	} catch (Exception x) {
		throw new JsonException(
				"Provider " + DEFAULT_PROVIDER + " could not be instantiated: " + x,
				x);
	}
}

[glassfishrobot]

@glassfishrobot Commented
stincmale said:
I also wonder if there is a place where I could find a roadmap with dates so I could see when version with fix (1.0.5 I suppose) may be released?

[glassfishrobot]

@glassfishrobot Commented
kchung said:
The code is not synchronized, but it is harmless. There is no race condition here. The worst that can happen is more than one instance of default JsonProvider may be created, and two threads may get different instances. I don't see how it can fail.

I should make defaultJsonProvide volatile, though.

[glassfishrobot]

@glassfishrobot Commented
stincmale said:

The code is not synchronized, but it is harmless

No, JLS do allows your code to return null from provider() method; also if it returns not null there is no guarantee that internals of the returned object will be seen correctly.

There is no race condition here

That's not true. There is both race condition and data race (see Race Condition vs. Data Race if you don't know the difference). As you said, "more than one instance of default JsonProvider may be created" - this is a race condition, because timing/ordering of operations affects behavior.

Now let's see if there is a data race. Let's refer to JLS:
JLS 17.4.5. Happens-before Order - "When a program contains two conflicting accesses that are not ordered by a happens-before relationship, it is said to contain a data race."
JLS 17.4.1. Shared Variables - "Two accesses to (reads of or writes to) the same variable are said to be conflicting if at least one of the accesses is a write."

In your code there are reads and writes of the field defaultJsonProvider and these actions aren't ordered with happens-before, so by definition there is a data rece.

I should make defaultJsonProvide volatile, though.

This will remove data race but race condition still will be there ("more than one instance of default JsonProvider may be created"). This won't be critical, but why you don't want to write more stable code?

I don't see how it can fail.

Yeah, yeah..., reminds me this Benign Data Races: What Could Possibly Go Wrong?

If you still think that your code is OK and harmless, I would suggest you to ask someone who you trust and who do understand Java memory model (yep, such programmers are rarity), or write a question to concurrency-interest mail group. But please don't commit code as specified in your second try.

[glassfishrobot]

@glassfishrobot Commented
kchung said:
The way defaultProvider is initialized lazily is known as "single check idiom" (Effective Java, Item 71). It may create more than one instance, but it IS harmless.

However, I've discovered a better way of doing this, which is known as "lazy initialization holder class idiom".

--- a/api/src/main/java/javax/json/spi/JsonProvider.java
+++ b/api/src/main/java/javax/json/spi/JsonProvider.java
@@ -71,7 +71,32 @@ public abstract class JsonProvider {
      */
     private static final String DEFAULT_PROVIDER
             = "org.glassfish.json.JsonProviderImpl";
-    private static ServiceLoader<JsonProvider> loader = ServiceLoader.load(JsonProvider.class);
+    private static final ThreadLocal<ServiceLoader<JsonProvider>> threadLoader =
+        new ThreadLocal<ServiceLoader<JsonProvider>>() {
+            @Override
+            protected ServiceLoader<JsonProvider> initialValue() {
+return ServiceLoader.load(JsonProvider.class);
+            }
+        };
+
+    //Lazy initialization holder class idiom +    private static class JsonProviderHolder {
+        static final JsonProvider defaultJsonProvider = initDefault();
+
+        static JsonProvider initDefault() {
+            try {
+Class<?> clazz = Class.forName(DEFAULT_PROVIDER);
+return (JsonProvider)clazz.newInstance();
+            } catch (ClassNotFoundException x) {
+throw new JsonException(
+        "Provider " + DEFAULT_PROVIDER + " not found", x);
+            } catch (Exception x) {
+throw new JsonException(
+        "Provider " + DEFAULT_PROVIDER + " could not be instantiated: " + x,
+        x);
+            }
+        }
+    }

     protected JsonProvider() {
     }
@@ -86,22 +111,12 @@ public abstract class JsonProvider {
      * @return a JSON provider
      */
     public static JsonProvider provider() {
-        Iterator<JsonProvider> it = loader.iterator();
+        Iterator<JsonProvider> it = threadLoader.get().iterator();
         if (it.hasNext()) {
             return it.next();
         }

-        try {
-            Class<?> clazz = Class.forName(DEFAULT_PROVIDER);
-            return (JsonProvider)clazz.newInstance();
-        } catch (ClassNotFoundException x) {
-            throw new JsonException(
-    "Provider " + DEFAULT_PROVIDER + " not found", x);
-        } catch (Exception x) {
-            throw new JsonException(
-    "Provider " + DEFAULT_PROVIDER + " could not be instantiated: " + x,
-    x);
-        }
+        return JsonProviderHolder.defaultJsonProvider;
     }

[glassfishrobot]

@glassfishrobot Commented
stincmale said:

The way defaultProvider is initialized lazily is known as "single check idiom" (Effective Java, Item 71). It may create more than one instance, but it IS harmless.

Effective Java tells us that field must be volatile for "single check idiom" (as I said, in this case we will not have a data race, but will have a harmless race condition; still the code is correctly synchronized and can be commited)

Without volatile modificator one must use a local variable and the reference must be of primitive (except long and double) or immutable type. So instead of

if (defaultJsonProvider != null) {
	return defaultJsonProvider;// counterintuitive, but JLS do allow this to return null if defaultJsonProvider isn't volatile
}

one must write

JsonProvider result = defaultJsonProvider;// use local variable, so defaultJsonProvider is read only once
if (result != null) {
	return result;// this can never return null
}

Effective Java refers to this idiom as "racy single-check idiom".

Code in the comment 24/Jun/14 9:30 PM is not a "single check idiom" because of the lack of volatile and not a "racy single-check idiom" because there is no local variable and JsonProvider is not immutable (only thread-safety is required by its specification).

I've discovered a better way of doing this, which is known as "lazy initialization holder class idiom"

Yep, this is a good alternative to "double-check idiom". I'm OK with it

[glassfishrobot]

@glassfishrobot Commented
kchung said:
Thanks for taking time helping me to get this right. I've learned much.

[glassfishrobot]

@glassfishrobot Commented
stincmale said:
You are welcome.

Is there a place where I could find a roadmap with dates so I could see when version with this fix (1.0.5 I suppose) may be released?

[glassfishrobot]

@glassfishrobot Commented
roman.grigoriadi said:
Looks like this fix actually introduced performance problems instead of eliminating them.

There is no need to cache ServiceLoader to ThreadLocal inside the API itself. Client application as well as JMH tests should cache JsonProvider instance instead if needed. JsonProvider should be thread safe and can be cached even between threads.

Jsonb runtime does such thing. When jsonb runtime instance is created JsonProvider instance is cached and reused between all threads. This change brings down performance of jsonb considerably. I am voting to revert the change.

[glassfishrobot]

@glassfishrobot Commented
stincmale said:
@roman.grigoriadi Could you please explain in more details why exactly this change brought down the performance of Jsonb?
Before change

public static JsonProvider provider() {
	ServiceLoader<JsonProvider> loader = ServiceLoader.load(JsonProvider.class);
	Iterator<JsonProvider> it = loader.iterator();
	...
}

After change

public static JsonProvider provider() {
	Iterator<JsonProvider> it = threadLoader.get().iterator();//threadLoader is ThreadLocal
	...
}

Are you saying that threadLoader.get() is slower than ServiceLoader.load(JsonProvider.class)? And if Jsonb actually caches the returned JsonProvider, then how this change could affect its performance?

[glassfishrobot]

@glassfishrobot Commented
roman.grigoriadi said:
Good question actually. I will elaborate more on the technical reason, but benchmarks on both jsonb and jsonp alone with provider cached seems to perform better without this change on my JMH tests runned with multiple threads. In both cases this threadlocal is accessed only during first initialization for each thread, so accessing thethreadlocal var should not be the cause.

Here is an example of such simple test: https://github.com/json-p/jsonp-jmh/blob/master/src/main/java/org/glassfish/json/JsonGeneratorTest.java