Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Register all polyglot java import classes for refective access #9997
Register all polyglot java import classes for refective access #9997
Changes from 5 commits
5cddb23
773af10
e70f53a
ee24418
6886b12
6f3bfad
e409b00
e9551b7
e9a55f0
5cc50de
8ab9362
da144be
c5f03b0
e08f747
8d1d8af
1be75e6
d36bc75
5a1743a
e8a3325
3db825c
d22357c
aaa5e9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the
Standard.Base
library needs 153 classes registered for reflection. Libs guys, @radeusgd, @AdRiley, @jdunkerley, @GregoryTravis - isn't that a bit too much?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every registered class, I have to register all its
public
:That may be too much - as not all these methods are really called by Enso code. As such, it might be better to use following pattern. Rather than calling org.w3c.dom.Element.getTagName() you could wrap the call into:
that way we would concentrate and select only the needed methods and the transitive closure of needed code computed by
native-image
would be smaller.If we don't create the wrapper methods then we'll be forced to
polyglot java import
additional classes like in 6f3bfad as the native image needs to know types of classes we want to invoke instance methods on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the list you provided, every of these classes seems needed to provide some of the functionality of the standard library. The std-lib provides a broad variety of functionalities so it seems expected it may have some dependencies. 153 does not seem that large to me.
I guess we may want to discuss whether we should split off some of these dependencies away from
Base
into separate libraries. I guess we could indeed do so, to keep a 'lighter' "core". IF we split off, I suspect most of our base workflows will for the time being import most of the separated parts anyway, as they are expected to be available for the users out of the box. So what could be the benefit of splitting?I expect @jdunkerley may have some opinions on whether we want to keep a 'big'
Base
library or make it more modularized.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be counting just the classes, but rather methods. The problem with current approach is that by using a method rich class like HttpClient we "bring in" all its methods (including for example such a megamorphic methods like
equals
,hashCode
andtoString
), while we are probably using just a few. Theallows us to select only the needed methods and the transitive closure of needed code computed by
native-image
would be smaller. Hence I'd like you to use this static method wrapper where possible forStandard
libraries to be eligible for native image compilation.Right now the
./runner
has 418MB (with Graal.js and GraalPython compiled in). By carefully selecting what goes into the image we might get to 410MB or maybe even below 400MB.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's like 5%? Seems like a lot of hassle and maintenance for reducing the image by that amount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is not going to be much.
On contrary, I see that (e.g. exactly specifying what methods are used) as a good coding and architecture practice regardless of the expected gain in the size of native executable. Way more responsible than importing everything and hoping someone makes the result efficient.
If this native compilation of libraries gets to production, we should make sure we squeeze
Standard
libraries to make them as much effective as possible.