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

Support subclass mocks on Graal VM. #2613

Merged
merged 1 commit into from Apr 19, 2022
Merged

Support subclass mocks on Graal VM. #2613

merged 1 commit into from Apr 19, 2022

Conversation

raphw
Copy link
Member

@raphw raphw commented Apr 7, 2022

Adds support for running Mockito on Graal native image using the (still experimental) experimental-class-define-support. Byte Buddy now picks up the Graal image code and creates more predicatable class files such that hashes during the JIT runtime and the image runtime match.

This leads to some minor restrictions, but generally this should not be a big issue. With future improvements in Graal, these restrictions might fade in the future.

As an example, with the mockto.jar that is produced in this PR:

#!/bin/bash
set -x
set -e
if [[ -z $1 ]]; then
  echo "Specify location of Graal VM as first argument"
  exit 1
fi
$1/bin/gu install native-image
rm -rf META-INF sample.build_artifacts.txt Sample.class Sample.java
echo "public class Sample {
  public static void main(String[] args) {
    Sample sample = org.mockito.Mockito.mock(Sample.class);
    org.mockito.Mockito.when(sample.m()).thenReturn(\"Hello Graal!\");
    System.out.println(sample.getClass());
    System.out.println(sample.m());
  }
  public String m() { return null; }
}" > Sample.java
wget -O bytebuddy.jar https://search.maven.org/remotecontent?filepath=net/bytebuddy/byte-buddy/1.12.9/byte-buddy-1.12.9.jar
#wget -O mockito.jar https://search.maven.org/remotecontent?filepath=org/mockito/mockito-core/XXX/mockito-core-XXXX.jar
wget -O objenesis.jar https://search.maven.org/remotecontent?filepath=org/objenesis/objenesis/3.2/objenesis-3.2.jar
export dependencies=bytebuddy.jar:mockito.jar:objenesis.jar:.
$1/bin/javac -cp $dependencies Sample.java
$1/bin/java -agentlib:native-image-agent=config-output-dir=META-INF/native-image,experimental-class-define-support -Dorg.graalvm.nativeimage.imagecode=agent -cp $dependencies Sample
$1/bin/native-image --no-fallback -cp $dependencies \
--rerun-class-initialization-at-runtime=\
net.bytebuddy.ClassFileVersion,\
net.bytebuddy.utility.dispatcher.JavaDispatcher,\
net.bytebuddy.utility.Invoker\$Dispatcher \
--initialize-at-build-time=\
net.bytebuddy.description.method.MethodDescription\$InDefinedShape\$AbstractBase\$ForLoadedExecutable,\
net.bytebuddy.description.type.TypeDescription\$AbstractBase,\
net.bytebuddy.description.type.TypeDescription\$ForLoadedType,\
net.bytebuddy.description.method.MethodDescription\$ForLoadedMethod,\
net.bytebuddy.implementation.bind.annotation.Argument\$BindingMechanic,\
net.bytebuddy.implementation.bind.annotation.Argument\$BindingMechanic\$1,\
net.bytebuddy.implementation.bind.annotation.Argument\$BindingMechanic\$2,\
net.bytebuddy.implementation.bind.annotation.Super\$Instantiation\$2 \
Sample
./sample

The setup is still a bit cluncky but Oracle is working on improving the process. There will likely be plugins for the tools in question to improve this.

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2022

Codecov Report

Merging #2613 (c0d7443) into main (8314824) will decrease coverage by 0.34%.
The diff coverage is 34.78%.

❗ Current head c0d7443 differs from pull request most recent head d23dc0e. Consider uploading reports for the commit d23dc0e to get more accurate results

@@             Coverage Diff              @@
##               main    #2613      +/-   ##
============================================
- Coverage     86.62%   86.27%   -0.35%     
+ Complexity     2762     2761       -1     
============================================
  Files           314      314              
  Lines          8237     8268      +31     
  Branches       1017     1026       +9     
============================================
- Hits           7135     7133       -2     
- Misses          842      869      +27     
- Partials        260      266       +6     
Impacted Files Coverage Δ
.../creation/bytebuddy/SubclassBytecodeGenerator.java 74.25% <28.57%> (-10.26%) ⬇️
...al/creation/bytebuddy/SubclassInjectionLoader.java 54.71% <28.57%> (-2.73%) ⬇️
...ito/internal/creation/bytebuddy/ModuleHandler.java 21.42% <33.33%> (-1.40%) ⬇️
.../main/java/org/mockito/internal/util/Platform.java 84.61% <33.33%> (-4.58%) ⬇️
.../internal/creation/bytebuddy/MockMethodAdvice.java 80.40% <100.00%> (+0.11%) ⬆️
...al/creation/bytebuddy/InlineBytecodeGenerator.java 84.48% <0.00%> (-0.14%) ⬇️
...internal/creation/bytebuddy/BytecodeGenerator.java 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8314824...d23dc0e. Read the comment docs.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't had the chance to look at the implementation yet, but would it be possible to add a mockito-graal test project to our regression suite? That would help me with reviewing the implementation as well. Thanks!

@raphw
Copy link
Member Author

raphw commented Apr 8, 2022

I looked at it, but this would require running on Graal and installing the native extension. I am not sure how realistic this is and I would want to avoid downloading a GB for each build. It also requires two execution circles.

For those reasons, I have decided against it in Byte Buddy and rather rely on manual tests so long as the general setup is still autotested. I was hoping that the Graal ecosystem spawns some more mature testing utilities in the future.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Only a couple of nits and clarifying questions. Other than that LGTM!

GraalImageCode.getCurrent().isDefined()
&& !features.interfaces.contains(Serializable.class)
? CompoundList.of(
new ArrayList<Type>(features.interfaces),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we maybe use a Set here, so that we can skip the contains check for Serializable? E.g. on Graal we always add Serializable to the Set and then we create the CompoundList of that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to replace it with a sorted set, actually as a different order would cause a different class file and hash and fail the Graal run.

"%s$%s$%s",
typeName,
"MockitoMock",
GraalImageCode.getCurrent().isDefined()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason we only want this on Graal? I can imagine we would want this for others as well, to keep everything consistent?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The very theoretical situation would be a OSGi setup where multiple modules of Mockito wanted to create a mock for the same class loader. It's unlikely but might trigger a regression. The question is if the stable name justifies this theoretical issue.

@@ -35,6 +33,8 @@ public void pure_mockito_should_not_depend_bytecode_libraries() throws Exception
"org.mockito.internal.creation.instance.DefaultInstantiatorProvider");
pureMockitoAPIClasses.remove(
"org.mockito.internal.creation.instance.ObjenesisInstantiator");
pureMockitoAPIClasses.remove(
"org.mockito.internal.creation.instance.UnsafeInstantiatorStrategy");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we no longer need this, as this particular class isn't included in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this stems from an intermediate version to work around something that is now fixed in Graal.

@raphw raphw force-pushed the graal-support branch 2 times, most recently from d95060f to 1a9cbfe Compare April 19, 2022 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants