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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,18 @@ install: true

jobs:
include:
- stage: checkstyle
script: mvn checkstyle:checkstyle
- stage: copyright
script: bash etc/copyright.sh
- stage: install-yasson
script: mvn -U -C -Pstaging clean install
# - stage: run-jmh
# script:
# - cd yasson-jmh
# - mvn clean install
# - java -jar target/yasson-jmh.jar -t 1 -f 2
- stage: checkstyle
script: mvn checkstyle:checkstyle
- stage: copyright
script: mvn glassfish-copyright:check
- stage: tck-run
script: bash tck.sh
script: bash etc/tck.sh


22 changes: 22 additions & 0 deletions etc/copyright.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/bin/bash -x
#
# 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
#

die(){ echo "${1}" ; exit 1 ;}

readonly RESULT_FILE="copyright-check.txt"

mvn -q org.glassfish.copyright:glassfish-copyright-maven-plugin:copyright \
> ${RESULT_FILE} || (cat ${RESULT_FILE}; die "Error running the Maven command")

grep -i "copyright" ${RESULT_FILE} \
&& die "COPYRIGHT ERROR" || echo "COPYRIGHT OK"
5 changes: 3 additions & 2 deletions tck.sh → etc/tck.sh
100755 → 100644
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ GF_BUNDLE_URL="central.maven.org/maven2/org/glassfish/main/distributions/glassfi
TCK_NAME=jsonb-tck
TCK_VERSION=1.0.1

export TCK_HOME=`pwd`"/target-tck"
export YASSON_HOME=`pwd`
export TCK_HOME=${YASSON_HOME}"/target-tck"
rm -r ${TCK_HOME}
mkdir ${TCK_HOME}
cd ${TCK_HOME}
Expand All @@ -33,7 +34,7 @@ wget -q --no-cache ${GF_BUNDLE_URL} -O latest-glassfish.zip
echo "Exporting downloaded GlassFish"
unzip -qq ${TCK_HOME}/latest-glassfish.zip -d ${TCK_HOME}

cp -a ${TCK_HOME}/target/yasson.jar ${TCK_HOME}/glassfish5/glassfish/modules/yasson.jar
cp -a ${YASSON_HOME}/target/yasson.jar ${TCK_HOME}/glassfish5/glassfish/modules/yasson.jar

cd ${TS_HOME}/bin

Expand Down
14 changes: 12 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,18 @@
<groupId>org.glassfish.copyright</groupId>
<artifactId>glassfish-copyright-maven-plugin</artifactId>
<executions>
<!--Copyright goal does not fail when copyright is not properly updated,
but prints out which file has incorrect copyright.-->
<execution>
<id>run-copyright</id>
<id>print-copyright</id>
<goals>
<goal>copyright</goal>
</goals>
<phase>validate</phase>
</execution>
<!--Check goal does not print out incorrect copyright files, but fails when there are errors.-->
<execution>
<id>check-copyright</id>
<goals>
<goal>check</goal>
</goals>
Expand Down Expand Up @@ -510,7 +520,7 @@
<plugin>
<groupId>org.glassfish.copyright</groupId>
<artifactId>glassfish-copyright-maven-plugin</artifactId>
<version>2.2</version>
<version>2.3</version>
<configuration>
<templateFile>etc/copyright.txt</templateFile>
<excludeFile>etc/copyright-exclude.txt</excludeFile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ private Type resolveTypeArg(Type adapterTypeArg, Type adapterType) {
return ReflectionUtils.resolveTypeArguments((ParameterizedType) adapterTypeArg, adapterType);
} else if (adapterTypeArg instanceof TypeVariable) {
return ReflectionUtils
.resolveItemVariableType(new RuntimeTypeHolder(null, adapterType), (TypeVariable<?>) adapterTypeArg);
.resolveItemVariableType(new RuntimeTypeHolder(null, adapterType), (TypeVariable<?>) adapterTypeArg, true);
} else {
return adapterTypeArg;
}
Expand Down
46 changes: 41 additions & 5 deletions src/main/java/org/eclipse/yasson/internal/JsonBindingBuilder.java
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

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

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

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

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;
}
}
41 changes: 24 additions & 17 deletions src/main/java/org/eclipse/yasson/internal/ReflectionUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public static Class<?> resolveRawType(RuntimeTypeInfo item, Type type) {
return getRawType(resolveType(item, type));
}
}

/**
* Resolve a type by item.
* If type is a {@link TypeVariable} recursively search {@link AbstractItem} for resolution of typevar.
Expand All @@ -111,10 +111,14 @@ public static Class<?> resolveRawType(RuntimeTypeInfo item, Type type) {
* @return resolved type
*/
public static Type resolveType(RuntimeTypeInfo item, Type type) {
return resolveType(item, type, true);
}

private static Type resolveType(RuntimeTypeInfo item, Type type, boolean warn) {
if (type instanceof WildcardType) {
return resolveMostSpecificBound(item, (WildcardType) type);
return resolveMostSpecificBound(item, (WildcardType) type, warn);
} else if (type instanceof TypeVariable) {
return resolveItemVariableType(item, (TypeVariable<?>) type);
return resolveItemVariableType(item, (TypeVariable<?>) type, warn);
} else if (type instanceof ParameterizedType && item != null) {
return resolveTypeArguments((ParameterizedType) type, item.getRuntimeType());
}
Expand All @@ -130,33 +134,36 @@ public static Type resolveType(RuntimeTypeInfo item, Type type) {
*/
public static Optional<Type> resolveOptionalType(RuntimeTypeInfo info, Type type) {
try {
return Optional.of(resolveType(info, type));
return Optional.of(resolveType(info, type, false));
} catch (RuntimeException e) {
return Optional.empty();
}
}

/**
* Resolve a bounded type variable type by its wrapper types.
* Resolution could be done only if a compile time generic information is provided, either:
* by generic field or subclass of a generic class.
*
* @param whether or not to log a warning message when bounds are not found
* @param item item to search "runtime" generic type of a TypeVariable.
* @param typeVariable type to search in item for, not null.
* @return Type of a generic "runtime" bound, not null.
*/
public static Type resolveItemVariableType(RuntimeTypeInfo item, TypeVariable<?> typeVariable) {
static Type resolveItemVariableType(RuntimeTypeInfo item, TypeVariable<?> typeVariable, boolean warn) {
if (item == null) {
//Bound not found, treat it as an Object.class
LOGGER.warning(Messages.getMessage(MessageKeys.GENERIC_BOUND_NOT_FOUND,
typeVariable,
typeVariable.getGenericDeclaration()));
if (warn) {
LOGGER.warning(Messages.getMessage(MessageKeys.GENERIC_BOUND_NOT_FOUND,
typeVariable,
typeVariable.getGenericDeclaration()));
}
return Object.class;
}

//Embedded items doesn't hold information about variable types
if (item instanceof EmbeddedItem) {
return resolveItemVariableType(item.getWrapper(), typeVariable);
return resolveItemVariableType(item.getWrapper(), typeVariable, warn);
}

ParameterizedType wrapperParameterizedType = findParameterizedSuperclass(item.getRuntimeType());
Expand All @@ -165,12 +172,12 @@ public static Type resolveItemVariableType(RuntimeTypeInfo item, TypeVariable<?>
Type foundType = search.searchParametrizedType(wrapperParameterizedType, typeVariable);
if (foundType != null) {
if (foundType instanceof TypeVariable) {
return resolveItemVariableType(item.getWrapper(), (TypeVariable<?>) foundType);
return resolveItemVariableType(item.getWrapper(), (TypeVariable<?>) foundType, warn);
}
return foundType;
}

return resolveItemVariableType(item.getWrapper(), typeVariable);
return resolveItemVariableType(item.getWrapper(), typeVariable, warn);
}

/**
Expand Down Expand Up @@ -314,23 +321,23 @@ private static ParameterizedType findParameterizedSuperclass(Type type) {
* @param wildcardType Wildcard type.
* @return The most specific type.
*/
private static Type resolveMostSpecificBound(RuntimeTypeInfo item, WildcardType wildcardType) {
private static Type resolveMostSpecificBound(RuntimeTypeInfo item, WildcardType wildcardType, boolean warn) {
Class<?> result = Object.class;
for (Type upperBound : wildcardType.getUpperBounds()) {
result = getMostSpecificBound(item, result, upperBound);
result = getMostSpecificBound(item, result, upperBound, warn);
}
for (Type lowerBound : wildcardType.getLowerBounds()) {
result = getMostSpecificBound(item, result, lowerBound);
result = getMostSpecificBound(item, result, lowerBound, warn);
}
return result;
}

private static Class<?> getMostSpecificBound(RuntimeTypeInfo item, Class<?> result, Type bound) {
private static Class<?> getMostSpecificBound(RuntimeTypeInfo item, Class<?> result, Type bound, boolean warn) {
if (bound == Object.class) {
return result;
}
//if bound is type variable search recursively for wrapper generic expansion
Type resolvedBoundType = bound instanceof TypeVariable ? resolveType(item, bound) : bound;
Type resolvedBoundType = bound instanceof TypeVariable ? resolveType(item, bound, warn) : bound;
Class<?> boundRawType = getRawType(resolvedBoundType);
//resolved class is a subclass of a result candidate
if (result.isAssignableFrom(boundRawType)) {
Expand Down
Empty file.
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,24 @@ public ObjectSerializer(CurrentItem<?> wrapper, Type runtimeType, ClassModel cla

@Override
protected void serializeInternal(T object, JsonGenerator generator, SerializationContext ctx) {
final PropertyModel[] allProperties = ((Marshaller) ctx).getMappingContext().getOrCreateClassModel(object.getClass())
.getSortedProperties();
for (PropertyModel model : allProperties) {
try {
marshallProperty(object, generator, ctx, model);
} catch (Exception e) {
throw new JsonbException(Messages.getMessage(MessageKeys.SERIALIZE_PROPERTY_ERROR, model.getWriteName(),
object.getClass().getCanonicalName()), e);
Marshaller context = (Marshaller) ctx;
try {
if (context.addProcessedObject(object)) {
final PropertyModel[] allProperties = context.getMappingContext().getOrCreateClassModel(object.getClass())
.getSortedProperties();
for (PropertyModel model : allProperties) {
try {
marshallProperty(object, generator, context, model);
} catch (Exception e) {
throw new JsonbException(Messages.getMessage(MessageKeys.SERIALIZE_PROPERTY_ERROR, model.getWriteName(),
object.getClass().getCanonicalName()), e);
}
}
} else {
throw new JsonbException(Messages.getMessage(MessageKeys.RECURSIVE_REFERENCE, object.getClass()));
}
} finally {
context.removeProcessedObject(object);
}
}

Expand Down
15 changes: 7 additions & 8 deletions src/main/resources/META-INF/native-image/native-image.properties
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
################################################################################
#
# 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 v1.0 and Eclipse Distribution License v. 1.0
# which accompanies this distribution.
# The Eclipse Public License is available at http://www.eclipse.org/legal/epl-v10.html
# and the Eclipse Distribution License is available at
# 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.
#
# Contributors:
# Tomas Langer
################################################################################
# SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause
#

# This adds command line options to native image tool
Args=-H:IncludeResourceBundles=yasson-messages
47 changes: 47 additions & 0 deletions src/test/java/org/eclipse/yasson/adapters/model/Chain.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* 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.adapters.model;

public class Chain {

private String name;
private Chain linksTo;
private Foo has;

public Chain(String name) {
this.name = name;
}

public Chain() {
}

public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public Chain getLinksTo() {
return linksTo;
}
public void setLinksTo(Chain linksTo) {
this.linksTo = linksTo;
}
public Foo getHas() {
return has;
}
public void setHas(Foo has) {
this.has = has;
}

}
Loading