Optimize new Gson()

[MukjepScarlet]

Optimizes new Gson()

closes #2863

[eamonnmcmanus]

I'm not really convinced here. It might have been better if this had been the API from the outset, but now we have 17 years of client code that uses new Gson(). So I think it would be very disruptive to deprecate that, and if we don't then we have two different ways of doing the same thing.

However, it's true that every invocation of new Gson() ends up doing quite a lot of work in order to construct an exactly equivalent list of factories every time. I believe we could improve that situation as follows:

• Replace the package-private Gson constructor that has 21 parameters by a constructor that takes a GsonBuilder parameter.
• Have a static final GsonBuilder that the public Gson() constructor passes to the new Gson(GsonBuilder).
• Somehow cache the List<TypeAdapterFactory> corresponding to this builder, and reuse it when the new constructor sees that it is being called a second time from the Gson() constructor.

The last item is potentially controversial, since it does mean that some memory would end up being retained even when there are no active Gson instances.

Whether or not we do the last item, I think the first two would be worthwhile, if only because it would eliminate the duplication whereby the default value for every Gson property is specified in both the Gson() constructor and the GsonBuilder instance initializers.

[MukjepScarlet]

I will drop Gson.DEFAULT and try to make the Default constructor share more contents.

[MukjepScarlet]

I need help about Java 9 modules...

[Marcono1234]

I need help about Java 9 modules...

It looks like there is something wrong with how the adapters are looked up. It is trying to use the reflective adapter for types such as java.lang.Double.

I have made some changes to this code here and proposed them as MukjepScarlet#1; that also fixes the test failures. If you are fine with them, you can merge them and they will directly appear in this PR here then.

But @MukjepScarlet, could you please edit the title of this PR here (click the "Edit" button at the top right) and rename it to something like "Reduce overhead of default Gson instances" or similar? The current title ("add Gson.DEFAULT") is misleading because the PR is not actually doing this anymore.

[Marcono1234]

@eamonnmcmanus, what do you think about the current state of this PR?

[eamonnmcmanus]

@eamonnmcmanus, what do you think about the current state of this PR?

The outline looks correct, but I think there is a subtle change in behaviour. When I ran this against Google's internal tests, I saw failures that looked like this:

Error occurred during initialization of VM
java.lang.BootstrapMethodError: java.lang.IllegalStateException: getSystemClassLoader cannot be called during the system class loader instantiation
        at com.google.gson.internal.ConstructorConstructor.newMapConstructor(ConstructorConstructor.java:381)
        at com.google.gson.internal.ConstructorConstructor.newDefaultImplementationConstructor(ConstructorConstructor.java:328)
        at com.google.gson.internal.ConstructorConstructor.get(ConstructorConstructor.java:137)
        at com.google.gson.internal.bind.MapTypeAdapterFactory.create(MapTypeAdapterFactory.java:146)
        at com.google.gson.Gson.getAdapter(Gson.java:349)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.createBoundField(ReflectiveTypeAdapterFactory.java:205)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.getBoundFields(ReflectiveTypeAdapterFactory.java:396)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.create(ReflectiveTypeAdapterFactory.java:165)
        at com.google.gson.Gson.getAdapter(Gson.java:349)
        at com.google.gson.Gson.fromJson(Gson.java:1079)
        at com.google.gson.Gson.fromJson(Gson.java:981)
        at com.google.gson.Gson.fromJson(Gson.java:891)
        at com.google.gson.Gson.fromJson(Gson.java:828)
        at [... Google-internal stuff ...]
        at java.lang.reflect.Constructor.newInstanceWithCaller(java.base@25/Constructor.java:499]
        at java.lang.reflect.Constructor.newInstance(java.base@25/Constructor.java:483]
        at java.lang.ClassLoader.initSystemClassLoader(java.base@25/ClassLoader.java:1889]
        at java.lang.System.initPhase3(java.base@25/System.java:1993]
Caused by: java.lang.IllegalStateException: getSystemClassLoader cannot be called during the system class loader instantiation
        at java.lang.ClassLoader.getSystemClassLoader(java.base@25/ClassLoader.java:1849]
        at java.lang.invoke.MethodHandles$Lookup.canBeCached(java.base@25/MethodHandles.java:4022]
        at java.lang.invoke.MethodHandles$Lookup.linkMethodHandleConstant(java.base@25/MethodHandles.java:3999]
        at java.lang.invoke.MethodHandleNatives.linkMethodHandleConstant(java.base@25/MethodHandleNatives.java:603]
        at com.google.gson.internal.ConstructorConstructor.newMapConstructor(ConstructorConstructor.java:381)
        at com.google.gson.internal.ConstructorConstructor.newDefaultImplementationConstructor(ConstructorConstructor.java:328)
        at com.google.gson.internal.ConstructorConstructor.get(ConstructorConstructor.java:137)
        at com.google.gson.internal.bind.MapTypeAdapterFactory.create(MapTypeAdapterFactory.java:146)
        at com.google.gson.Gson.getAdapter(Gson.java:349)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.createBoundField(ReflectiveTypeAdapterFactory.java:205)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.getBoundFields(ReflectiveTypeAdapterFactory.java:396)
        at com.google.gson.internal.bind.ReflectiveTypeAdapterFactory.create(ReflectiveTypeAdapterFactory.java:165)
        at com.google.gson.Gson.getAdapter(Gson.java:349)
        at com.google.gson.Gson.fromJson(Gson.java:1079)
        at com.google.gson.Gson.fromJson(Gson.java:981)
        at com.google.gson.Gson.fromJson(Gson.java:891)
        at com.google.gson.Gson.fromJson(Gson.java:828)
        at [... more Google-internal stuff ...]

I guess this code is running with -Djava.system.class.loader=something, but in any case it is clearly triggering a code path that was not triggered before. I haven't had time to investigate further.

[Marcono1234]

This PR does change () -> new C() to C::new in ConstructorConstructor.java (which is not actually needed for this PR).

If you revert those ConstructorConstructor.java changes, do the tests pass then? Would be quite interesting though that lambda vs. method reference makes a difference.

[MukjepScarlet]

I'm interesting about that too. If you think the change of ConstructorConstructor is not needed (or should be in another PR), I'll revert it here.

[MukjepScarlet]

I think this is the reason because stuffs like TreeMap::new will access getSystemClassLoader, but in the test it's not ready. This might need further test.

[Marcono1234]

I think I am able to reproduce this locally:

1. For testing (but don't include it in this PR), add gson/src/main/java/com/google/gson/SystemClassLoader.java with this content:

   package com.google.gson;
   
   import java.util.Map;
   import java.util.concurrent.atomic.AtomicReference;
   
   public class SystemClassLoader extends ClassLoader {
       private static final AtomicReference<Object> value = new AtomicReference<>();
   
       public SystemClassLoader(ClassLoader parent) {
           super(parent);
           Object object = new Gson().fromJson("{}", Map.class);
           value.set(object);
       }
   
       @SuppressWarnings("SystemOut")
       public static void main(String[] args) {
           System.out.println(value.get());
           System.out.println("done");
       }
   }

2. Build the Gson JAR

   mvn clean package --projects gson "-Dmaven.test.skip"
   

3. Run with the custom system class loader

   java -cp ./gson/target/gson-2.13.3-SNAPSHOT.jar "-Djava.system.class.loader=com.google.gson.SystemClassLoader" com.google.gson.SystemClassLoader
   

Fails with the same stack trace as above. Replacing the method references in ConstructorConstructor fixes it.
Though to be sure, would be good @eamonnmcmanus if you could double-check if that is indeed the same issue.

In that case maybe we could indeed replace the method references in ConstructorConstructor with lambdas again. Though this seems somewhat brittle and I am wondering if there are other not-yet discovered issues which could occur when using Gson from within a custom system class loader.

Interestingly this only seems to affect LinkedTreeMap::new, maybe because it is a reference to a non-JDK class?
You can reproduce this with this custom class loader (and same build / run steps as above):

public class SystemClassLoader extends ClassLoader {
    public SystemClassLoader(ClassLoader parent) {
        super(parent);
        List<Supplier<?>> unused = Arrays.asList(
            ArrayList::new,
            LinkedHashSet::new,
            TreeSet::new,
            ArrayDeque::new,
            // LinkedTreeMap::new,
            () -> new LinkedTreeMap<>(),
            LinkedHashMap::new,
            TreeMap::new,
            ConcurrentHashMap::new,
            ConcurrentSkipListMap::new
        );
    }

    @SuppressWarnings("SystemOut")
    public static void main(String[] args) {
        System.out.println("done");
    }
}

Edit: It seems the reason why this issue only occurs for ...

• Gson's LinkedTreeMap: the MethodHandles code only checks the system class loader if the class loader of the class is non-null; for JDK classes it is null because they are loaded by the bootstrap class loader
• a method reference (::new) but not a lambda: for the lambda the compiler seems to generate a private snythetic method, and the method handle then refers to that, but the caching logic in MethodHandles ignores non-public members

At least that is my understanding of it. But this also seems to highly depend on JDK and compiler implementation details. But given that usage of lambdas here worked before apparently, that seems to be a reasonable solution, albeit brittle. Though using Gson in general from within the constructor of a custom system class loader seems to be brittle.

[eamonnmcmanus]

Thanks! This is basically what I had in mind. But it looks like there are a lot of changes in here that are not really related to the important change, which is eliminating duplication between Gson and GsonBuilder. Would it possible to make it so that that is the only thing that changes?

[Marcono1234]

But it looks like there are a lot of changes in here that are not really related to the important change

Sorry, might also have been my fault to some extent due to the changes I had proposed to @MukjepScarlet.
I guess the changes to EnumTypeAdapter.java and ConstructorConstructor.java are unrelated and could be done in a separate PR indeed.

The changes to SqlTypesSupport.java and LongSerializationPolicy.java are related and probably nice to have since they slightly simplify creation of the default factories, but maybe could be omitted / be performed in a subsequent PR?

The change to TypeAdapters.java is closely related:
These adapters and factories are currently created within Gson.java, but with these changes, GsonBuilder instead of Gson now creates the default so it would make sense to move those adapters and factories out of Gson.java either way, and TypeAdapters.java seems suitable?
But maybe that could be done in a predecessor PR then? So first move the adapters and adapter factories to TypeAdapters in one PR, and then base this PR here on it.

@eamonnmcmanus, is that what you had in mind?

And @MukjepScarlet what do you think about this? Do you think this is a reasonable way to split this PR, and would you be able to do that?

[MukjepScarlet]

Splitting the PR is reasonable, as certain changes within it were deliberately intended to serve as separate prerequisite PR. I have no objection. @Marcono1234

[eamonnmcmanus]

(I'm still looking for a PR that has no changes other than the ones needed to remove the duplication between Gson and GsonBuilder. Not that those other changes are necessarily bad, they just shouldn't be here.)

[Marcono1234]

@eamonnmcmanus, most of the changes have been moved by @MukjepScarlet into #2951, which is kind of a prerequisite for this PR here now I think. If possible could you please have a look at that PR?

Once that is (hopefully) merged and this PR here is updated, the remaining changes here should be only¹ about the improvements for the Gson default instance.

Footnotes

1. @MukjepScarlet, the EnumTypeAdapter change here is still unrelated to the Gson default instance changes though. Maybe it would be best to omit it from this PR here? ↩

[MukjepScarlet]

@eamonnmcmanus I think this is ready for review now.

[eamonnmcmanus]

Sorry for the long delay here. It looks like this PR is now focused on the one change of reducing duplication in Gson and GsonBuilder. I have one very minor comment, but it can potentially be addressed in a follow-up.

[eamonnmcmanus]

I think it would be a bit cleaner to set these values through the public API of GsonBuilder.

[MukjepScarlet]

Yes, better readability, just like how we use it outside of this package.

[eamonnmcmanus]

Thanks again, @MukjepScarlet, for seeing this PR through to completion! I think it's a great simplification of the code.