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

WELD-2764 Method invokers, first impl #2864

Merged
merged 20 commits into from Oct 25, 2023
Merged

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Jul 25, 2023

JIRA - WELD-2764

Draft for the prototype of invokeable methods in Weld.
CI failures are expected due to SNAPSHOT deps.
CI should now be fine , there are no longer any snapshot dependencies and there is added profile that allows CDI and interceptors API patching in WFLY. All tests should pass.

The impl is based on Java MethodHandles.

How to try out:

  • Checkout and build (mvn clean install) the CDI branch with invokeable methods API proposal
  • Checkout and build this Weld branch (mvn clean install -DskipTests)
  • You now have Weld 5.1.2-SNAPSHOT with draft of the impl
    • If you wish to see use cases, take a look at the tests in this PR

Looking at performance:

The implementation uses MethodHandles and as such is very efficient. The difference in direct method invocation versus invocation via Invoker becomes negligible the moment method does anything non-trivial (basically anything else than returning a constant). Currently, I have been playing with some microbenchmarks using JMH to test this. A WIP can be seen over in this repo and branch. If you want to run that:

  • Check out the repo and switch to the invokableMethods branch
  • mvn clean install
  • ./run-benchmarks.sh -v "5.1.2-SNAPSHOT" -b "InvokableMethodBenchmarkNoLookup InvokableMethodBenchmarkSimpleBeanInvocation InvokableMethodBenchmarkLookup"

Known TODOs:

  • Support for build compatible implementations (currently only works via portable extensions)
    • BCEs are now supported as well; there is basic test coverage as the impl is more or less a matter of delegation
  • Support exception transformers returning arbitrary value (which is something method handles don't allow out of the box)
    • You can now return any return value from exception transformers; this requires some juggling on Weld side, but ultimately works
  • Properly support lookup with qualifiers
    • The PR now supports this and has respective test coverage
  • Proper exception handing setup
  • Deployment validation of lookups
    • This should also include deployment resolution of lookups and storing them in some form that will speed up repeated resolutions when invoking the method
  • Validation of invokable methods during invoker creation
    • After removal of @Invokable we now need to add verification that happens when you attempt to create an invoker for any given method; for instance validate that the method is non-private
  • Tests/changes for invokeable methods with generics?

@manovotn manovotn force-pushed the invokableMethods branch 2 times, most recently from 0ba2566 to 313d464 Compare August 1, 2023 10:29
@manovotn manovotn force-pushed the invokableMethods branch 2 times, most recently from e3ddebe to b8fa638 Compare September 6, 2023 17:05
manovotn and others added 17 commits September 14, 2023 16:35
An instance of `CleanupActions` must exist for each _invocation_,
not for each _invoker_. This requires some creative juggling of
method handle arguments, which is what this commit does.
This currently doesn't help anything, but it should be possible
to optimize out `CleanupActions` instantiation completely when
we know that it is never used.
This required moving the invoker construction logic from `InvokerImpl`
into the `AbstractInvokerBuilder`, which is arguably a better place anyway.
The `InvokerImpl` looks fairly minimal now, as it should.
@manovotn
Copy link
Contributor Author

Added changes to support recent adjustment in CDI PR - removal of @Invokable and the need to hold metadata about which methods are invokable for each bean class. You can now attempt to create invoker for any method of given bean (and some should fail if you do)

This also reduces the volume of this PR from 96 changes files to just 68 :)

@manovotn
Copy link
Contributor Author

I guess all of the in-container tests fail because we aren't patching the CDI JAR in WFLY dist; will take a closer look on Mon as to whether we can improve that until we have WFLY version which consumes the expected artifact.

@manovotn manovotn changed the title Invokable methods Method invokers, first impl Oct 25, 2023
@manovotn manovotn changed the title Method invokers, first impl WELD-2764 Method invokers, first impl Oct 25, 2023
@manovotn manovotn marked this pull request as ready for review October 25, 2023 09:50
@manovotn
Copy link
Contributor Author

I've created a follow up JIRA issue capturing leftover TODOs from this one and targetting Alpha2.

@manovotn manovotn merged commit 6c82f5b into weld:master Oct 25, 2023
12 checks passed
@manovotn manovotn deleted the invokableMethods branch October 25, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants