Skip to content

cache JsonProvider result in JsonBindingBuilder #363

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.

Sign up for GitHub

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

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

cache JsonProvider result in JsonBindingBuilder #363

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f91e4b
cache JsonProvider result in JsonBindingBuilder
Simulant87 Nov 24, 2019
e85434e
StackOverflowError on recursive references (#359)
jbescos Nov 24, 2019
e13dd73
Copyright travis job improved (#365)
Verdent Nov 25, 2019
8776082
Do not log WARN message when optional generic bound is not found
aguibert Nov 24, 2019
8e25cf5
Generated Yasson jar has not been properly moved to TCK tests
Verdent Nov 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 41 additions & 5 deletions src/main/java/org/eclipse/yasson/internal/JsonBindingBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

package org.eclipse.yasson.internal;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

import javax.json.bind.Jsonb;
Expand All @@ -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

@aguibert aguibert Nov 24, 2019

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

@aguibert aguibert Nov 24, 2019

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

private Map<String, Object> configCache = null;
private JsonProvider providerCache = null;

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

@aguibert aguibert Nov 24, 2019

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

this.config = config;
return this;
}
}

@Override
public JsonbBuilder withProvider(JsonProvider jsonpProvider) {
this.provider = jsonpProvider;
return this;
synchronized (this) {
this.provider = jsonpProvider;
return this;
}
}

/**
Expand All @@ -58,6 +67,33 @@ public Optional<JsonProvider> getProvider() {

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

@aguibert aguibert Nov 24, 2019

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

if (bindingCache != null
&& configEqualsCachedConfig()
&& providerEqualsCachedProvider()) {
return bindingCache;
}
JsonBinding jsonBinding = new JsonBinding(this);
cacheCurrentConfiguration(jsonBinding);
return jsonBinding;
}
}

private boolean configEqualsCachedConfig() {
return (configCache != null && config != null && configCache.equals(config.getAsMap()))
|| (config == null && configCache == null);
}

private boolean providerEqualsCachedProvider() {
return (providerCache != null && providerCache.equals(provider))
|| (provider == null && providerCache == null);
}

private void cacheCurrentConfiguration(JsonBinding jsonBinding) {
bindingCache = jsonBinding;
if (config != null) {
configCache = new HashMap<>(config.getAsMap());
}
providerCache = provider;
}
}
72 changes: 72 additions & 0 deletions src/test/java/org/eclipse/yasson/internal/JsonBindingBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright (c) 2019 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0,
* or the Eclipse Distribution License v. 1.0 which is available at
* http://www.eclipse.org/org/documents/edl-v10.php.
*
* SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause
*/

package org.eclipse.yasson.internal;

import org.glassfish.json.JsonProviderImpl;
import org.junit.jupiter.api.Test;

import javax.json.bind.Jsonb;
import javax.json.bind.JsonbConfig;

import static org.junit.jupiter.api.Assertions.*;

public class JsonBindingBuilderTest {

@Test
public void testMultipleCallsToBuildWithoutChangesReturnTheSameInstance() {
Copy link
Member

@aguibert aguibert Nov 24, 2019

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

JsonBindingBuilder builder = new JsonBindingBuilder();

Jsonb jsonb1 = builder.build();
Jsonb jsonb2 = builder.build();

assertSame(jsonb1, jsonb2);
}

@Test
public void testMultipleCallsToBuildWithEqualConfigReturnTheSameInstance() {
JsonBindingBuilder builder = new JsonBindingBuilder();
JsonbConfig config = new JsonbConfig();

Jsonb jsonb1 = builder.build();
builder.withConfig(config);
Jsonb jsonb2 = builder.build();

assertSame(jsonb1, jsonb2);
}


@Test
public void testMultipleCallsToBuildWithChangedConfigReturnNotTheSameInstance() {
Copy link
Member

@aguibert aguibert Nov 24, 2019

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

JsonBindingBuilder builder = new JsonBindingBuilder();
JsonbConfig config = new JsonbConfig();
builder.withConfig(config);

Jsonb jsonb1 = builder.build();
config.withStrictIJSON(true);
Jsonb jsonb2 = builder.build();

assertNotSame(jsonb1, jsonb2);
}

Copy link
Member

@aguibert aguibert Nov 24, 2019

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


@Test
public void testMultipleCallsToBuildWithChangedProviderReturnNotTheSameInstance() {
JsonBindingBuilder builder = new JsonBindingBuilder();

Jsonb jsonb1 = builder.build();
builder.withProvider(new JsonProviderImpl());
Jsonb jsonb2 = builder.build();

assertNotSame(jsonb1, jsonb2);
}
}