Skip to content
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.

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

cache JsonProvider result in JsonBindingBuilder #363

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Simulant87
Copy link
Contributor

@Simulant87 Simulant87 commented Nov 24, 2019

implement ticket #333

Copy link
Member

@aguibert aguibert left a comment

Choose a reason for hiding this comment

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

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

@@ -25,17 +27,24 @@
public class JsonBindingBuilder implements JsonbBuilder {
private JsonbConfig config = new JsonbConfig();
private JsonProvider provider = null;
private JsonBinding bindingCache = null;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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())


@Override
public JsonbBuilder withConfig(JsonbConfig config) {
this.config = config;
return this;
synchronized (this) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -58,6 +67,33 @@ public JsonbConfig getConfig() {

@Override
public Jsonb build() {
return new JsonBinding(this);
synchronized (this) {
Copy link
Member

Choose a reason for hiding this comment

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

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));

public class JsonBindingBuilderTest {

@Test
public void testMultipleCallsToBuildWithoutChangesReturnTheSameInstance() {
Copy link
Member

Choose a reason for hiding this comment

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

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()



@Test
public void testMultipleCallsToBuildWithChangedConfigReturnNotTheSameInstance() {
Copy link
Member

Choose a reason for hiding this comment

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

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


assertNotSame(jsonb1, jsonb2);
}

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

Verdent commented Nov 25, 2019

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.

jbescos and others added 4 commits November 26, 2019 19:56
Fix StackOverflowError on recursive references

Signed-off-by: Jorge Bescos Gascon <[email protected]>
* Copyright plugin version fix.
Printing of incorrect copyrights improved.

Signed-off-by: David Kral <[email protected]>

* Suggestions of aguibert according to copyright implemented.

Signed-off-by: David Kral <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants