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

refactor(java): replace Guava's TypeToken with self-made #1553

Merged
merged 20 commits into from
Apr 26, 2024

Conversation

Munoon
Copy link
Contributor

@Munoon Munoon commented Apr 21, 2024

What does this PR do?

Removes Guava's TypeToken usages and replaces it with a hand-made TypeToken implementation.
To be honest, this is mostly a copy-paste of Guava's TypeToken with removing some unnecessary stuff. This implementation can be improved and optimized.

Related issues

#1113
#1273

# Conflicts:
#	NOTICE
#	java/fury-core/src/main/java/org/apache/fury/builder/CodecUtils.java
#	java/fury-core/src/main/java/org/apache/fury/builder/MetaSharedCodecBuilder.java
#	java/fury-core/src/main/java/org/apache/fury/resolver/ClassResolver.java
#	java/fury-core/src/test/java/org/apache/fury/FuryInitPerf.java
#	java/fury-format/src/main/java/org/apache/fury/format/encoder/ArrayEncoderBuilder.java
#	java/fury-format/src/main/java/org/apache/fury/format/encoder/MapEncoderBuilder.java
#	java/fury-format/src/main/java/org/apache/fury/format/encoder/RowEncoderBuilder.java
@Munoon Munoon changed the title Remove guava type token refactor(java): replace Guava's TypeToken with self-made Apr 21, 2024
@chaokunyang
Copy link
Collaborator

Great! The CI has some failure:
image

Could you fix this?

@chaokunyang
Copy link
Collaborator

Fury has a org.apache.fury.type.GenericType, which can be taken as an example. We may don't need the complete feature of guava TypeToken

@Munoon
Copy link
Contributor Author

Munoon commented Apr 23, 2024

Fury has a org.apache.fury.type.GenericType, which can be taken as an example. We may don't need the complete feature of guava TypeToken

I had implemented only methods, that was marked with a TODO comment. As I understand, all of them are currently used. However, I'm not sure if all the code, that was copied from Guava were actually used, but I've decided, that it is more safe to copy everything.

@Munoon
Copy link
Contributor Author

Munoon commented Apr 23, 2024

@chaokunyang could you please help with GraalVM build? I'm not sure how it should be fixed...

@LiangliangSui
Copy link
Contributor

Hi @Munoon , you can append the following to the end of native-image.properties and it should resolve this CI error

org.apache.fury.reflect.Types$ClassOwnership, \
org.apache.fury.reflect.Types$ClassOwnership$1, \
org.apache.fury.reflect.Types$ClassOwnership$2

@chaokunyang
Copy link
Collaborator

chaokunyang commented Apr 23, 2024

@chaokunyang could you please help with GraalVM build? I'm not sure how it should be fixed...

You need to add org.apache.fury.reflect.Types$ClassOwnership,org.apache.fury.reflect.Types$ClassOwnership$2,org.apache.fury.reflect.Types$ClassOwnership$1 to src/main/resources/META-INF/native-image/org.apache.fury/fury-core/native-image.properties.

Those classes needs to be initialized at graalvm build time since it's used by Fury init and creation.

}

/** Returns true if this type is a supertype of the given {@code type}. */
public final boolean isSupertypeOf(Type type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we change to:

 public final boolean isSupertypeOf(Type type, boolean generic) {
     if (generic) {
        throw new UnsupportedException(xxx);
     }

In this way, the code will be much simpler.

Currently Fury didn't use covariant geneics. Such checks is complicated and increate the maintain cost. If we do need it, we can add it back in future

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we don't need the generic covariant checks. Many methods such as canonicalizeWildcardType can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

when generic is noe used ,this can be simplified into :

public final boolean isSupertypeOf(TypeRef<?> type) {
    return getRawType().isAssignableFrom(type.getRawType());
  }

@chaokunyang
Copy link
Collaborator

Hi @Munoon , thanks for this great and hard work. I left some comments, LGTM overall. Thanks again. This is not an easy work

@Munoon
Copy link
Contributor Author

Munoon commented Apr 24, 2024

Hi @Munoon , thanks for this great and hard work. I left some comments, LGTM overall. Thanks again. This is not an easy work

Appreciate this. I'll try to resolve all those comments in a coming days.

# Conflicts:
#	java/fury-core/src/main/java/org/apache/fury/builder/BaseObjectCodecBuilder.java
#	java/fury-core/src/main/java/org/apache/fury/builder/CodecBuilder.java
#	java/fury-core/src/main/java/org/apache/fury/builder/CompatibleCodecBuilder.java
#	java/fury-core/src/main/java/org/apache/fury/builder/ObjectCodecBuilder.java
#	java/fury-core/src/main/java/org/apache/fury/codegen/Expression.java
#	java/fury-core/src/main/java/org/apache/fury/codegen/ExpressionVisitor.java
#	java/fury-core/src/main/java/org/apache/fury/meta/ClassDef.java
#	java/fury-core/src/main/java/org/apache/fury/resolver/ClassResolver.java
#	java/fury-core/src/main/java/org/apache/fury/serializer/ObjectSerializer.java
#	java/fury-core/src/main/java/org/apache/fury/serializer/StructSerializer.java
#	java/fury-core/src/main/java/org/apache/fury/type/Descriptor.java
#	java/fury-core/src/main/java/org/apache/fury/util/ReflectionUtils.java
Copy link
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

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

LGTM, do you have any further updates? @Munoon

@Munoon
Copy link
Contributor Author

Munoon commented Apr 26, 2024

LGTM, do you have any further updates? @Munoon

I didn't start yet processing this comment: #1553 (comment)
But if you think, that it is not required - lets go as it is not to break anything. We can optimize this in a further PRs.

Also, I'm not sure, but I think that generics are used there when using maps and list.

@chaokunyang
Copy link
Collaborator

chaokunyang commented Apr 26, 2024

LGTM, do you have any further updates? @Munoon

I didn't start yet processing this comment: #1553 (comment) But if you think, that it is not required - lets go as it is not to break anything. We can optimize this in a further PRs.

Also, I'm not sure, but I think that generics are used there when using maps and list.

The compiler used by Fury doesn't generics, so Fury ignore generics in many places. Let's merge this first, we can optimize other parts after we removed guava.

@chaokunyang chaokunyang merged commit a003624 into apache:main Apr 26, 2024
32 checks passed
chaokunyang added a commit that referenced this pull request Apr 27, 2024
## What does this PR do?

add guava derive comments

## Related issues

#1553 


## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/incubator-fury/issues/new/choose)
describing the need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?


## Benchmark

<!--
When the PR has an impact on performance (if you don't know whether the
PR will have an impact on performance, you can submit the PR first, and
if it will have impact on performance, the code reviewer will explain
it), be sure to attach a benchmark data here.
-->
chaokunyang added a commit that referenced this pull request Jul 13, 2024
## What does this PR do?

This PR moves `org.apache.fury.reflect.Types` into TypeRef. Types is
used in Fury type system, by merge `org.apache.fury.reflect.Types`, we
can reduce ambiguation in fury type system

## Related issues
#1553 
#1690 

## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/fury/issues/new/choose) describing the
need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?


## Benchmark

<!--
When the PR has an impact on performance (if you don't know whether the
PR will have an impact on performance, you can submit the PR first, and
if it will have impact on performance, the code reviewer will explain
it), be sure to attach a benchmark data here.
-->
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.

4 participants