cache JsonProvider result in JsonBindingBuilder

[Simulant87]

implement ticket #333

[aguibert]

hi @Simulant87, thanks for having a go at this enhancement. There are a few things that I would like to change in order to widen the scope of caching to make it cover more realistic scenarios

[aguibert]

A few things on the binding cache here:

1. The binding cache needs to be static so that it is global (i.e. across multiple different instances of JsonBindingBuilder).
2. We should allow more than one configuration of a JsonbBinding to be cached. Consider creating a composite key class (that combines provider and config) so we can store cached instances in a Map<JsonbKey,Jsonbinding>. This would also allow us to eagerly initialize the map and make it final
3. whatever data structure we use to store the JsonBinding instances needs to be a weak reference so we don't cause claassloader leaks when this is used in an app server environment where the server-side Yasson code has references to application-side classes that may be restarted/recycled multiple times during the same instance of the server-side code

[aguibert]

the JsonbKey class could look like this:

private static class JsonbKey {
  private final JsonbConfig config;
  private final JsonProvider provider;
  public JsonbKey(JsonbConfig config, JsonProvider provider) {
    // set final fields
  }
  // implement equals() and hashCode()
}

In order for this to work, we will need to properly compare equality for Jsonb instances which can be done by doing configA.getAsMap().equals(configB.getAsMap())

[aguibert]

synchronization shouldn't be necessary here or in withProvider. Instead, if we store cached instances in a map we can handle all concurrency within the build method

[aguibert]

Instead of synchronizing here, we could take advantage of some concurrent map operations. For example:

JsonbKey currentKey = new JsonbKey(config, provider);
return jsonbCache.computeIfAbsent(currentKey, key -> new JsonBinding(this));

[aguibert]

lets write tests that are more similar to how our users interact with Yasson/JSONB. In this case, you are doing new JsonBindingBuilder() which is from a Yasson internal package and users should never be using.
Instead, lets do things like JsonbBuilder.create() or JsonbBuilder.newBuilder().build()

[aguibert]

lets also test that multiple instances with config changed in the same way are equal. In this case, 2 instances that both do config.withStrictIJSON(true); should be equal

[aguibert]

lets also add a test that verifies 2 instances that both specify the same Adapter instance are the same, likewise another test that verifies 2 instances with different instances of the same Adapter class are different

[aguibert]

Another complexity I just realized is that the Jsonb interface has a close() method on it, which will complicate the ability to share instances. I don't have a clean solution to this problem at the moment, but have 2 ideas in mind (neither of which are ideal)

1. Store a reference count in our JsonBinding implementation and the count goes up once for every new shared instance that gets passed out, and the count goes down once each time close() gets called on an instance. When the count gets to 0 the instance is fully closed. This approach seems nice but it would render the performance optimization useless for the typical "get -> use -> close" scenario
2. Allow an instance to be "reopened" after it is closed. This would be fine for the non-CDI case where close() is a no-op, but is not possible in CDI environments where beans are destroyed upon close

[Verdent]

Hi @Simulant87 , your build is failing on copyright job in our pipeline. Currently there is an issue with that on our side. See PR #365 . Please when this PR is merged, just rebase on it and everything should be fine.