JsonPatchImpl.getPointer should use a cached JsonProvider instance to create pointers.

[JohnTimm]

The getPointer method in JsonPatchImpl uses: Json.createPointer(...) to create JsonPointer instances.

As mentioned in other GitHub issues, this is inefficient due to ServiceLoader invocation on every call. Unfortunately, there isn't a mechanism for clients using JsonPatch to replace or override this behavior.

A cached instance of JsonProvider should be used within JsonPatchImpl or there should be an alternative way to create JsonPatch objects that allow the client to pass in a Provider instance. The workaround I am considering is to re-implement the JsonPatch.apply method using a cached provider to create Pointer instances.

[keilw]

I think that applies to more than just the getPointer method. If JsonProvider had some sort of cache (similar to what we did in JSR 385) then it would also require something to reload or invalidate the cache, or an explicit setProvider of some sort. Sounds doable, but probably for a new version e.g. under Jakarta EE 9. Where either by default or in a Multi-Release JAR the Java 9+ version could even try to use the Module system.

[leadpony]

I measured throughput of JsonPatch#apply() method using the first example in RFC 6902 with JMH.

Reference Implementation

Benchmark                  Mode  Cnt     Score    Error  Units
JsonPatchBenchmark.apply  thrpt   25  8092.975 ± 59.420  ops/s

Joy

Benchmark                  Mode  Cnt        Score        Error  Units
JsonPatchBenchmark.apply  thrpt   25  3822741.473 ± 632635.898  ops/s

The former is more than 470 times slower than the latter. It is just a problem of poor implementation, not a problem of the API.

[JohnTimm]

Another option would be to follow the pattern used in DocumentBuilderFactory and TransformerFactory and let the client explicitly set the implementation to use via a factory method and/or a System property

[keilw]

Well the JSR 385 ServiceProvider allows to set an active provider explicitly or pick the desired one by name. However, as @leadpony mentioned, there could be other aspects where the former RI lacks optimization and unless Oracle or Eclipse Foundation and the contributors to Jakarta EE wish to beef up the Glassfish one, I don't see any problem with other compatible implementations being faster. That's the idea and it means they have an advantage to be picked over others ;-)

[JohnTimm]

Per the comments above, would something like this be quick win to improve overall performance of the library (at least as it relates to invoking ServiceLoader)?

public final class Json {
    private static final JsonProvider PROVIDER = JsonProvider.provider();

    private Json() {
    }

    // ...

    public static JsonArrayBuilder createArrayBuilder() {
        return PROVIDER.createArrayBuilder();
    }

    public static JsonArrayBuilder createArrayBuilder(JsonArray array) {
        return PROVIDER.createArrayBuilder(array);
    }

    public static JsonArrayBuilder createArrayBuilder(Collection<?> collection) {
        return PROVIDER.createArrayBuilder(collection);
    }

    public static JsonObjectBuilder createObjectBuilder() {
        return PROVIDER.createObjectBuilder();
    }

    public static JsonObjectBuilder createObjectBuilder(JsonObject object) {
        return PROVIDER.createObjectBuilder(object);
    }

    public static JsonObjectBuilder createObjectBuilder(Map<String, Object> map) {
        return PROVIDER.createObjectBuilder(map);
    }

    public static JsonPointer createPointer(String jsonPointer) {
        return PROVIDER.createPointer(jsonPointer);
    }

    public static JsonPatchBuilder createPatchBuilder() {
        return PROVIDER.createPatchBuilder();
    }

    public static JsonPatchBuilder createPatchBuilder(JsonArray array) {
        return PROVIDER.createPatchBuilder(array);
    }

    public static JsonPatch createPatch(JsonArray array) {
        return PROVIDER.createPatch(array);
    }

    public static JsonPatch createDiff(JsonStructure source, JsonStructure target) {
        return PROVIDER.createDiff(source, target);
    }

    public static JsonMergePatch createMergePatch(JsonValue patch) {
        return PROVIDER.createMergePatch(patch);
    }

    public static JsonMergePatch createMergeDiff(JsonValue source, JsonValue target) {
        return PROVIDER.createMergeDiff(source, target);
    }

    // ...

}

or an alternative:

public final class Json {
    private static JsonProvider provider = null;

    private Json() {
    }

    // ...

    public static synchronized void setProvider(JsonProvider provider) {
        Json.provider = provider;
    }

    private static JsonProvider provider() {
        if (provider == null) {
            // current behavior
            return JsonProvider.provider();
        }
        // use the client's implementation
        provider;
    }

    // ...

    public static JsonNumber createValue(long value) {
        return provider().createValue(value);
    }
}

[leadpony]

@JohnTimm Is Json in above code is actually javax.json.Json? If so, please read the comment by @lukasj in Issue #80. The API should not cache any service provider.

[JohnTimm]

Yes, that is the javax.json.Json class. Per Issue #80, I think that the option of allowing the client to set the provider would satisfy the requirement. I think the class could be refactored so that it defaults to the current behavior and allows an alternative IFF setProvider is invoked with a non-null argument.

[leadpony]

Sorry but I cannot fully understand your "or alternatively:" code above. The provider variable seems a member variable of Json instance. Would you please show me the omitted part? For example, how does Json.createValue() static method refer to the provider variable?

[JohnTimm]

@leadpony Apologies, provider should have been declared static. I have updated the "alternate" solution above.

[leadpony]

Application A wants to use JSON-P implementation X. Application B wants to use JSON-P implementation Y. Both are running on the same application server which loads the JSON-P API with its own class loader. I do not think your idea really satisfies this requirement.

[leadpony]

The biggest API design flaw in JSON-P is that the facade class Json is not instantiated when used by applications. In JSON-B:

Jsonb jsonb = JsonbBuilder.create();
jsonb.doSomethingUseful();
jsonb.doAnotherThingUseful();
...

The instance jsonb can be cached by application and this avoids any performance penalty around service loader, which JSON-P suffers.

[JohnTimm]

I was thinking about the "allow the client to set the provider" part of the requirement and not about it in terms of multiple class loaders. To address that issue I think we could use something like ContextClassLoaderLocal (from Apache Commons and others) to manage multiple provider instances each associated with a specific class loader.

Modeled after: https://github.com/javaee/metro-saaj/blob/master/saaj-ri/src/java/com/sun/xml/messaging/saaj/soap/ContextClassloaderLocal.java

public final class Json {
    private static final Map<ClassLoader, JsonProvider> providers = Collections.synchronizedMap(new WeakHashMap<>());

    private Json() {
    }

    // ...

    /**
     * Allows the client to set the JsonProvider instance to use on a per
     * context class loader basis.
     * 
     * @param provider JsonProvider instance
     */
    public static void setProvider(JsonProvider provider) {
        providers.put(getContextClassLoader(), provider);
    }

    private static JsonProvider provider() {
        return providers.getOrDefault(getContextClassLoader(), JsonProvider.provider());
    }

    private static ClassLoader getContextClassLoader() {
        return AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() {
            public ClassLoader run() {
                try {
                    return Thread.currentThread().getContextClassLoader();
                } catch (SecurityException e) {
                }
                return null;
            }
        });
    }

    // ...

    public static JsonNumber createValue(long value) {
        return provider().createValue(value);
    }
}

[keilw]

Turning the facade into a non-static instance class would break backward-compatibility, so I doubt that is easy to do. However the idea of a builder similar to JSON-B could be worth a thought for future Jakarta EE versions.

[keilw]

It may feel a bit silly, but we could call a new concrete class Jsonp ;-)

[JohnTimm]

I think with the setProvider method and a cache that is ClassLoader specific we can maintain backwards compatibility and get the performance improvements we are seeking.

[JohnTimm]

I measured throughput of JsonPatch#apply() method using the first example in RFC 6902 with JMH.

Reference Implementation

Benchmark                  Mode  Cnt     Score    Error  Units
JsonPatchBenchmark.apply  thrpt   25  8092.975 ± 59.420  ops/s

Joy

Benchmark                  Mode  Cnt        Score        Error  Units
JsonPatchBenchmark.apply  thrpt   25  3822741.473 ± 632635.898  ops/s

The former is more than 470 times slower than the latter. It is just a problem of poor implementation, not a problem of the API.

@leadpony Is this benchmark available somewhere? I'd like to run it with my recent changes.

[lukasj]

service lookups are avoided where possible through #244