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

Improvements to MemberBox #438

Merged
merged 1 commit into from
May 15, 2018
Merged

Improvements to MemberBox #438

merged 1 commit into from
May 15, 2018

Conversation

szegedi
Copy link
Contributor

@szegedi szegedi commented May 15, 2018

Fixes #429 by allowing serialization of MemberBox.delegateTo.

It also adds some more improvements enabled by the fact that in modern Java, we now have java.lang.reflect.Executable as a unifying type for both Method and Constructor. Using it reduces the need to typecheck and cast and it also allows unified access to parameter types and vararg flag, so they no longer need to be duplicated in the MemberBox.

Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

Just a minor comment on the "memberObject" -- can you look.

At some point we might also want to look and see if we get any functionality or performance advantages for using MethodHandle for these things rather than reflection.

transient Class<?>[] argTypes;
transient Object delegateTo;
transient boolean vararg;
private transient Executable memberObject;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this final? I'm asking because some of the blogs talk about JVM optimizations for reflection (and MethodHandles, which we don't use yet) that only happen when they're final.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it can't be both transient and final as then it couldn't be assigned upon deserialization.

@gbrail
Copy link
Collaborator

gbrail commented May 15, 2018

Of course -- thanks.

I often wish that Rhino didn't have serialization. But it does, so we need to leave it like this.

@gbrail
Copy link
Collaborator

gbrail commented May 15, 2018

OK -- thanks!

@gbrail gbrail merged commit cd19d61 into master May 15, 2018
@szegedi szegedi deleted the memberbox branch May 23, 2018 15:42
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