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): Streamline ClassResolver responsibilities #1417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LiangliangSui
Copy link
Contributor

To reduce the complexity of the ClassResolver class, we mainly do the following two things:

  1. Move ClassId related responsibilities into ClassIdAllocator class.

  2. Move registeredId2ClassInfo into ExtRegistry static inner class since they are content related.

To reduce the complexity of the `ClassResolver` class, we
mainly do the following two things:

1. Move ClassId related responsibilities into ClassIdAllocator class.

2. Move `registeredId2ClassInfo` into `ExtRegistry` static inner
class since they are content related.

Signed-off-by: LiangliangSui <coolsui.coding@gmail.com>
private final Fury fury;
private ClassInfo[] registeredId2ClassInfo = new ClassInfo[] {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

registeredId2ClassInfo is in the critical path, we'd better to keep it here instead of extRegistry, which incur another pointer indirection.

@chaokunyang
Copy link
Collaborator

Hi @LiangliangSui , thanks for this work. We're standardlizing Fury xlang serialization protocol in #1413 , which may change class resolver too, especially the class id part. Can we hold this until the xlang serialization protocol are merged, so we don't have to refactor ClassResolver again.

The type id is lift to the first-class concept in #1413, we do need to strealine it from class resolver.

@LiangliangSui
Copy link
Contributor Author

Ok, we keep this PR until the xlang serialization protocol is merged.

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