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

feat(java): add set serializer for concurrent set #1616

Merged
merged 11 commits into from
May 11, 2024

Conversation

MrChang0
Copy link
Contributor

@MrChang0 MrChang0 commented May 9, 2024

What does this PR do?

we use Sets.newConcurrentHashSet() to create set, with lastest guava version it use ConcurrentHashMapKeySetView and old version it use Collections.newSetFromMap(map).

remove CopyOnWriteArrayListSerializer from native-image.properties. #1614 forget delete.

Related issues

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

@MrChang0 MrChang0 requested a review from chaokunyang as a code owner May 9, 2024 13:21
@Override
public Set<?> read(MemoryBuffer buffer) {
final Map<?, Boolean> map = (Map<?, Boolean>) fury.readRef(buffer);
final Set<?> set = Collections.newSetFromMap(Collections.emptyMap());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just invoke Collections.newSetFromMap(map)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just invoke Collections.newSetFromMap(map)

constructor must be a empty map.

}

@Override
public Collection newCollection(MemoryBuffer buffer) {
Copy link
Collaborator

@chaokunyang chaokunyang May 10, 2024

Choose a reason for hiding this comment

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

I just found that this method should not be used if collection codegen is false.
We can just implement serialization like this:

    @Override
    public void write(MemoryBuffer buffer, Collection value) {
      Object fieldValue = Platform.getObject(value, offset);
      fury.writeRef(buffer, fieldValue);
    }

    @Override
    public Collection read(MemoryBuffer buffer) {
      final Map map = (Map) fury.readRef(buffer);
      return (Collection) Collections.newSetFromMap(map);
    }

Sorry for our document, we didn't make it clear about how to implement a new collection serializer.

Actually, the org.apache.fury.serializer.collection.AbstractCollectionSerializer#write should check codegen is true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take this way, but Collections.newSetFromMap(map) must use empty map(the check is in constructor method), so I have to use Platform.putObject to set data.
I need learn more about codegen to make sure code is right.

return value;
public Set<?> read(MemoryBuffer buffer) {
final Map map = (Map) fury.readRef(buffer);
final Set set = Collections.newSetFromMap(Collections.emptyMap());
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

final Map map = (Map) fury.readRef(buffer);
return Collections.newSetFromMap(map);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about:

final Map map = (Map) fury.readRef(buffer);
return Collections.newSetFromMap(map);

In JDK17

  SetFromMap(Map<E, Boolean> map) {
      if (!map.isEmpty())
          throw new IllegalArgumentException("Map is non-empty");
      m = map;
      s = map.keySet();
  }

that's why I can't do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thanks. If only allow users to create set first, then add values.

If so, the serialization should be simplified.

Here is the serialization code:

public boolean add(E e) { return m.put(e, Boolean.TRUE) == null; }

All map values should not be serialized. And map will be empty, so there is not shared reference for this map.

Your previous implementation seems better, only need to set codegen to true. So fury will generate serialization code for this set. In onCollectonWrite, you write map class, and newCollection, you read class and create map from it. You can get serializer for that map, and it must be a AbstractMapSerializer, then you can use AbstractMapSerializer#newMap to create the empty map, and build the set from it.

Current implementation is OK for me too. The default implementation in Fury will serializer whole map too.

We can merge this PR first, and create another PR to optimize it if you like .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to save map classInfo, It looks likes set serializer and less reflection operate. anyway tomorrow is workday, I will finish it.

@MrChang0 MrChang0 requested a review from chaokunyang May 10, 2024 14:12
@MrChang0
Copy link
Contributor Author

what fuck discussions

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

return (Set<?>) set;
public Collection onCollectionWrite(MemoryBuffer buffer, Set<?> value) {
final Map<?, Boolean> map = (Map<?, Boolean>) Platform.getObject(value, MAP_FIELD_OFFSET);
fury.getClassResolver().writeClassAndUpdateCache(buffer, map.getClass());
Copy link
Collaborator

Choose a reason for hiding this comment

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

UpdateCache is unnecessary, next written item will be different types. Other part looks great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine, I careless.

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.

2 participants