MethodHandle predictions/risks

[lukesandberg]

The method handle implementation will be a huge improvement over fast classes and reflection, but it is not without risks.

1. Spinning too many method handles consumes 'class meta space' and may create some surprising jit/garbage collection overheads

As we construct handles, the JVM will under the hood produce LambdaForms. These are aggresively 'interned' and optimized, but fundamentally are used to produce tiny .class files that host the generated methodhandle code. This whole process leverages a bunch of very well optimized and special cased codepaths in the JVM but still, dynamically creating lots of tiny classes has a cost. Possibly this will slow down 'startup', but it might also trigger some as yet unknown pathological behavior.

Mitigation idea: Investigate a more structured 'intermediate represenation' than just MethodHandle. This would allow us to generate fewer intermediate handles.
- a simple example is injecting a Collection<Provider<T>> (from a multibinder) into an @Provides method. We end up adding a bunch of silly wrapper handles. such that the resulting thing looks like

// from the delegate factory
var handle = MethodHandles.dropArguments(MethodHandles.constant(Object.class, <collection-constant>), 0, InternalContext.class, Dependency.class);
// from SingleParameterInjector
handle = MethodHandles.insertArguments(handle, 1, dependency);
handle = catchInternalProvisionExceptionAndRethrowWithSource(handle, dependency);
/// plus logic to attach it to the provider method using `filterArguments`

If we knew it was a constant, this could boil down to a insertArguments call for the provider method instead of adding parameters to ignore and then passing them. At the very least we could avoid the try catch.

The JIT can do these things already through the magic of inlining, but generating 6 tiny methods adds overhead. This is exactly why compilers use 'intermediate representations', so that they can leverage higher level abstractions to eliminate redundant work. In the above case we know the value will never fail and is never null and doesn't depend on context or dependency.

2. startup time regression

I had hoped to measure this on some Google applications but never did.

In theory with this approach we never allocate a FastClass (ignoring AOP and certain silly edge cases), so Guice.createInjector will be a bit faster, however those initial Provider.get calls will be more expensive as we produce and link MethodHandles for the first time. Due to caching and the way handles work internally, this will be amortized over time but still it is ambiguous what the impact will be overall.

If we do see startup time regressions (which from our perspective would be time from Guice.createinjector() to when the first Provider.get() call completes), then we could pursue solution #1 which should help, but beyond that there isn't a ton of ideas.

In the openjdk project they fixed a startup problem with string-concatenation via more hand-coded class generation: openjdk/jdk#20273

This could work for us as well but would be a large undertaking. Still with some profiling information about where time is spent it could make sense to take this approach for some specific cases (multibinders? perhaps the logic around tryStart/finish should be hand coded?), which would greatly decrease the effort. Still for that to be attractive it might make sense to wait for jdk24 so we can drop our asm dependency and leverage the ClassBuilding api.

3. Security! Modules! Unsafe!

Currently guice is using Unsafe in a few places to define classes and access package private methods in foreign modules/classloaders. The MethodHandle implementation grealy reduces our reliance on these things but increases our reliance on the setAccessible API.

So what to do!

Well we could continue to rely on setAccessible, and just tell users to add --add-opens` calls to address any issues, but that is lame, it would be better to create a fine grained capability API for users to use.

Binder.provideAccess(MethodHandles.Lookup lookup)

Then guice can use these lookups to unreflect Methods Constructors and Fields in other packages.

If we had that we could simply report errors when unreflect fails and advise users to pass us a Lookup object to address. Similarly we could use this to avoid the use of Unsafe in our back-compat fast-class API, since defineHiddenClass can target package private classes given an appropriate Lookup object.

So this API will probably be required even if the overall MethodHandle approach crashes and dies for some reason.

4. Eliminate the BytecodeGen option?

MethodHandles are certainly bytecode generation... but then again java.lang.reflect is implemented in terms of methodhandles already so any user setting this option is already using methodhandles. So i believe the only real usecase here is android? and possibly restricted runtime environments like app-engine?

A downside of the MethodHandle implementation is that a number of core classes got split into 3 paths (method-handles, fast-class, reflection), so it would be very nice to get that down to exactly 1 implementation to reduce testing and maintenance overhead.

I might poke at idea 1 above, but then again maybe not 👍 Just wanted to write this down so it isn't lost

[sameb]

Back from vacation! I'm going to use this Issue to iterate on closing the gaps w/ MethodHandles. Testing against the depot w/ the latest code is mostly clean. I was able to fix the "allocation leak test" by adding -Djava.lang.invoke.MethodHandle.CUSTOMIZE_THRESHOLD=0, which ensured the created LambdaForms were caught in the warmups instead of lazily later.

The current issue I'm trying to work through is related to bad code with custom providers that return the wrong type and cause ClassCastExceptions when trying to conform to constructor signatures (and possibly also method injection or other things).

This test encapsulates the issue:

 @Test
  @SuppressWarnings({"rawtypes", "unchecked"}) // Test requires incorrect types
  public void testConstructorBindingDependencyHasWrongType() {
    var module =
        new AbstractModule() {
          @Override
          protected void configure() {
            bind(String.class)
                .toProvider(
                    new Provider() {
                      @Override
                      public Integer get() {
                        return 1;
                      }
                    });
          }
        };
    Injector injector = Guice.createInjector(module);
    var pe = assertThrows(ProvisionException.class, () -> injector.getInstance(WantsString.class));
    assertThat(pe).hasCauseThat().isInstanceOf(ClassCastException.class);
  }

That test passes w/o MHs but fails w/ them. I tried changing ConstructorInjector.getConstructHandle to insert a new handle of catchUncaughtExceptionsInConstructorAndRethrow after the current proxy.getConstructHandle call, which solved that problem... but in turn broke testErrorPropagation for its "providerClass" variant. Without MHs, this path goes through ConstructorInjector.provision, which catches InvocationTargetException and wraps that. That ITE catch catches code within the constructor body (and ensuring the params conform to the signature), but not code within each SingleParameterInjector. I can't quite figure out how to replicate that with the MH architecture.

[sameb]

One thought here is to do some more explicit custom enforcement on "return type is expected type" on providers, with a ClassCast otherwise. That would be a more general possibly behavior-changing change, but likely a better one?

[sameb]

Indeed validating in ProviderInternalFactory seems to solve this. Will clean things up and test more broadly.

[lukesandberg]

We attempt to do exactly this in ProvidedByInternalFactory for @ProvidedBy bindings, but i wonder if the better place is in SingleParameterInjector?

guice/core/src/com/google/inject/internal/SingleParameterInjector.java

Line 59 in 73ed742

factory.getHandle(context, /* linked= */ false), 1, dependency),

that is where we produce a handle factory to bind to a specific Dependency so that might be a reasonable place? Im guessing there are other ways to trigger a type error like that. Unfortunately we cannot use Dependency since that triggers subtle issues with 'provider normalization', but perhaps we could add a getActualRawType method??.

for example,

  @Test
  @SuppressWarnings({"rawtypes", "unchecked"}) // Test requires incorrect types
  public void testConstructorBindingDependencyHasWrongType() {
    var module =
        new AbstractModule() {
          @Override
          protected void configure() {
            bind(String.class).to((Key) Key.get(Integer.class));
            bind(Integer.class).toInstance(2);
          }

        };
    Injector injector = Guice.createInjector(module);
    var pe = assertThrows(ProvisionException.class, () -> injector.getInstance(WantsString.class));
    assertThat(pe).hasCauseThat().isInstanceOf(ClassCastException.class);
  }

That triggers the same issue as far as i can tell but now via a Linked binding.

So... ideas

• we add a castReturnTo helper that throws an InternalProvisionException instead of ClassCastException?
• we change SingleParameterInjector to accept a concrete Class<?> to conform to, and push the logic there (but otherwise do the same thing)

[sameb]

Ugh, I didn't consider the linked binding issue. I have a pending change I'll submit Monday for the user Provider issue, and we can generalize it to all kinds of bindings afterwards. I feel like maybe linked bindings or toInstance ones could be done ahead of time -- at configuration -- since Guice surely knows the actual types of those even earlier. (It took a bit for the Provider one to land because of some depot cleanup.). After this issue, there's just a half dozen or so tests in the depot that consistently fail with MethodHandles, so will dig into what's going on there.

[lukesandberg]

you know, a simple solution here could be to just change the order of these two lines

guice/core/src/com/google/inject/internal/DefaultConstructionProxyFactory.java

Line 215 in 73ed742

handle = MethodHandles.filterArguments(handle, 0, typedHandles);

      var handle =
          InternalMethodHandles.catchErrorInConstructorAndRethrowWithSource(target, injectionPoint);
      handle = MethodHandles.filterArguments(handle, 0, typedHandles);

if instead it was

      var handle =MethodHandles.filterArguments(target, 0, typedHandles);
       handle = InternalMethodHandles.catchErrorInConstructorAndRethrowWithSource(handle, injectionPoint);

fixes it

I think there is a larger question about the quality of the error message here (e.g. it doesn't tell us what parameter is implicated, though that is a preexisting issue), but simply changing those two lines does fix both of these tests.

From examining the similar code paths for @provides methods we appear to already do this, but not in SingleMethodInjector

[lukesandberg]

Oh and I confirmed that this trick doesn't work with a toInstance binding 😅.

[sameb]

I've just uploaded #1897 which changes the core infra to check providers are always the right type, which I think is the right solution (regardless of MethodHandles).