Performance issue caused by the WeakReference in AbstractJaxbProvider

[seaswalker]

jersey/media/jaxb/src/main/java/org/glassfish/jersey/jaxb/internal/AbstractJaxbProvider.java

Line 284 in d9658aa

final WeakReference<JAXBContext> ref = jaxbContexts.get(type);

Hi everyone!

Since we're using a weak reference here instead of a strong reference, once a GC happens, it will cause the JAXBContext to be reinitialized, which is a pretty expensive operation.

In our service, by using a custom JaxbContextResolver, we managed to reduce the response time by about 100ms (P90). So, should we provide an option here to let users decide whether to use strong or weak references?(Or an LRU cache with a size limit) I understand that the purpose of using weak references might be to reduce memory usage, but in reality, many services are more sensitive to response time.

@Provider
public class JaxbContextResolver implements ContextResolver<JAXBContext> {

  private final Map<Class<?>, JAXBContext> contexts = new HashMap<>();
  private final Lock lock = new ReentrantLock();

  @Override
  public JAXBContext getContext(Class<?> type) {
    JAXBContext context;
    lock.lock();
    try {
      context = contexts.get(type);
      if (context == null) {
        context = JAXBContext.newInstance(type);
        contexts.put(type, context);
      }
    } catch (JAXBException ex) {
      throw new RuntimeException(ex);
    } finally {
      lock.unlock();
    }
    return context;
  }
}

[jbescos]

Try this other, it should be even faster:

@Provider
public class JaxbContextResolver implements ContextResolver<JAXBContext> {

  private final ConcurrentHashMap<Class<?>, JAXBContext> contexts = new ConcurrentHashMap<>();

  @Override
  public JAXBContext getContext(Class<?> type) {
    return contexts.computeIfAbsent(type, t -> {
      try {
        return JAXBContext.newInstance(t);
      } catch (JAXBException ex) {
        throw new RuntimeException(ex);
      }
    });
  }
}

Regarding your question, I'd prefer to set a limit of size in the map.

[seaswalker]

Thanks, I get what you're saying. But with the current logic, if something goes wrong, it'll be really tough for users to figure out the root cause. So, do you think using what we discussed earlier as the new default might be a better way to go?

@jbescos

[jbescos]

Thanks, I get what you're saying. But with the current logic, if something goes wrong, it'll be really tough for users to figure out the root cause. So, do you think using what we discussed earlier as the new default might be a better way to go?

@jbescos

The problem with the strong reference is that it could accumulate unused instances during the time. Maybe you improve the performance, but you will get a memory leak too.

We cannot apply that fix as a general solution. I think it is better that you have your custom class to deal with it.