Skip to content

JAVA-2799: Create GraalVM substitutions for Guava's UnsignedBytes #1443

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.

Sign up for GitHub

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

Closed
absurdfarce wants to merge 3 commits into from
Closed

JAVA-2799: Create GraalVM substitutions for Guava's UnsignedBytes #1443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fff3ea
Bits necessary to move Graal substitution for Guava's UnsignedBytes i…
May 19, 2020
5cf59dc
Marking Graal svm lib as having provided scope
May 20, 2020
00a9021
Fixing my bone-headed mistakes
May 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@
<artifactId>graal-sdk</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.graalvm.nativeimage</groupId>
<artifactId>svm</artifactId>
<scope>provided</scope>
</dependency>
Comment thread
absurdfarce marked this conversation as resolved.
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
Expand Down
37 changes: 37 additions & 0 deletions ...ss/driver/shaded/guava/common/primitives/LexicographicalComparatorHolderSubstitution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright DataStax, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.datastax.oss.driver.shaded.guava.common.primitives;

import com.oracle.svm.core.annotate.Alias;
import com.oracle.svm.core.annotate.RecomputeFieldValue;
import com.oracle.svm.core.annotate.Substitute;
import com.oracle.svm.core.annotate.TargetClass;
import java.util.Comparator;

@TargetClass(UnsignedBytes.LexicographicalComparatorHolder.class)
final class LexicographicalComparatorHolderSubstitution {

@Alias
@RecomputeFieldValue(kind = RecomputeFieldValue.Kind.FromAlias)
static Comparator<byte[]> BEST_COMPARATOR = UnsignedBytes.lexicographicalComparatorJavaImpl();

/* All known cases should be covered by the field substitution above... keeping this only
* for sake of completeness */
@Substitute
static Comparator<byte[]> getBestComparator() {
return UnsignedBytes.lexicographicalComparatorJavaImpl();
}
}
23 changes: 23 additions & 0 deletions .../com/datastax/oss/driver/shaded/guava/common/primitives/UnsafeComparatorSubstitution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright DataStax, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.datastax.oss.driver.shaded.guava.common.primitives;

import com.oracle.svm.core.annotate.Delete;
import com.oracle.svm.core.annotate.TargetClass;

@TargetClass(UnsignedBytes.LexicographicalComparatorHolder.UnsafeComparator.class)
@Delete
final class UnsafeComparatorSubstitution {}
7 changes: 6 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,11 @@
<artifactId>graal-sdk</artifactId>
<version>20.0.0</version>
</dependency>
<dependency>
<groupId>org.graalvm.nativeimage</groupId>
<artifactId>svm</artifactId>
<version>20.0.0</version>
Copy link
Copy Markdown
Contributor

@tomekl007 tomekl007 May 28, 2020

Choose a reason for hiding this comment

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

Is it safe to hardcode the version of the graal here?
The quarkus takes it from the env runtime variable.
What will happen if someone will try to compile it in earlier versions?

Copy link
Copy Markdown
Contributor Author

@absurdfarce absurdfarce May 29, 2020

Choose a reason for hiding this comment

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

This lib is only required to gain access to the annotations needed to implement the substitution. So long as the Graal native build respects those annotations there shouldn't be an issue. I've tested this with a toolchain going back to 19.2.1 and it worked without issue so we should be fine.

Note that Quarkus only supports 19.3.1 so even with 19.2.1 we'd be talking about users writing their own apps for Graal and not using Quarkus.

</dependency>
</dependencies>
</dependencyManagement>
<build>
Expand Down Expand Up @@ -693,7 +698,7 @@ limitations under the License.]]></inlineHeader>
<verbose>false</verbose>
<quiet>true</quiet>
<doclint>all,-missing</doclint>
<excludePackageNames>com.datastax.*.driver.internal*</excludePackageNames>
<excludePackageNames>com.datastax.*.driver.internal*,com.datastax.oss.driver.shaded.guava.common.primitives</excludePackageNames>
Copy link
Copy Markdown
Contributor Author

@absurdfarce absurdfarce May 19, 2020

Choose a reason for hiding this comment

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

Weirdly this was required... without this change Javadoc generation started failing the build:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  28.949 s
[INFO] Finished at: 2020-05-19T13:18:42-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.1:jar (attach-javadocs) on project java-driver-core: MavenReportException: Error while generating Javadoc: 
[ERROR] Exit code: 1 - javadoc: error - class file for com.datastax.oss.driver.shaded.guava.errorprone.annotations.CheckReturnValue not found
[ERROR] 
[ERROR] Command line was: /work/local/graalvm-ce-java8-19.3.1/jre/../bin/javadoc @options @packages
[ERROR] 
[ERROR] Refer to the generated Javadoc files in '/work/git/java-driver/core/target/apidocs' dir.
[ERROR] 
[ERROR] -> [Help 1]

I'm not sure where the reference to that class is coming from; it isn't introduced by the new dependency added in this change and isn't in the shaded Guava JAR.

Copy link
Copy Markdown
Contributor

@adutra adutra May 19, 2020

Choose a reason for hiding this comment

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

I think Graal generates the substituted class inside driver-core own classes, i.e. in driver-core/target/classes which is why many tools (javadoc, bundle) are picking it up.

I think you can actually exclude everything that is shaded here, just in case we generate some more substitutions in the future:

<excludePackageNames>com.datastax.*.driver.internal*,com.datastax.*.driver.shaded*</excludePackageNames>

<tags>
<!--
For custom `leaks-private-api` tag (apparently dash separators are not handled
Expand Down