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

Make some java platform modules optional #2910

Closed
XakepSDK opened this issue Oct 30, 2020 · 22 comments · Fixed by #2913
Closed

Make some java platform modules optional #2910

XakepSDK opened this issue Oct 30, 2020 · 22 comments · Fixed by #2913
Milestone

Comments

@XakepSDK
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Jackson module descriptor implicitly depends on these modules:

  1. requires java.desktop
  2. requires java.logging
  3. requires java.sql
  4. requires java.xml
    Can they become optional?

Describe the solution you'd like
Make these modules optional

  1. requires static java.desktop
  2. requires static java.logging
  3. requires static java.sql
  4. requires static java.xml
    This may require some code changes, since making module optional means that module may not be present at runtime.
    So code should check, if relevant classes are present.

Usage example
jlink and jpackage will produce smaller images. java.sql itself brings 10MB+ of dependencies, according to this google/gson#1707 (comment)

@XakepSDK XakepSDK added the to-evaluate Issue that has been received but not yet evaluated label Oct 30, 2020
@GedMarc
Copy link

GedMarc commented Oct 30, 2020

Remember that the benefits of going native imaging isn't so much the size as much as it is the new filesystem JRT, the new class loader, the encapsulated module paths and your 60% on average performance enhancements depending on your personal module structure.

That said, the reason why I left most of them as mandatory was to restrict the development to as small as possible with minimal impact. Java.desktop is enormous, the beanutils class is used from there and exists in the top most level databind. Java.logging will be in everywhere, that will always be present in your artefact.

My assumption was that size of the deployable would be the smallest concern granted all the other benefits you get by going modular, also when looking at what would usually be included in a module for a program that isn't hello world, it would be really weird to write something without java.sql

I'll highlight each use case tonight, and why I left them mandatory, but on an overall perspective, to me a deployable for an ee scale app of 89mb was acceptable. :)

@GedMarc
Copy link

GedMarc commented Oct 30, 2020

P.s. I did these implementations during the dev of guicedee, if you want to see total sizes of complete jlink apps try out www.guicedee.com xD

@cowtowncoder
Copy link
Member

As far as I remember, these modules are due to pre-existing support for certain types from pre-module era -- all included in JDK by default, so made sense at the time -- and typically cannot just be dropped without consulting user base. With bit of grepping I think:

  1. java.xml: for DOM types (Document, Element, mostly), XMLGregorianCalendar.
  2. java.sql: date/time units, java.sql.Date, java.sql.Time, java.sql.Timestamp
  3. java.beans: for annotations, actually, @java.beans.ConstructorProperties mainly, also @java.beans.Transient
  4. java.logging: this I don't know -- Jackson tries to avoid logging completely and core does none (Afterburner I think is one module that does minimal amount of logging) -- could probably be dropped but as @GedMarc said may be something you'll get from somewhere anyway

Of these, I think...

  1. java.xml: while it might seem jackson-dataformat-xml would make sense, that's not good fit -- these are datatypes, so might need jackson-datatype-jdk-xml or something
  2. java.sql: if I recall these types are surprisingly widely (mis/ab)used. No good home either
  3. java.beans: I think ConstructorProperties, too, is kind of widely used, unfortunately.
  4. java.logging: could probably drop?

On practical terms, I am not sure if anything can or should be done for Jackson 2.x; users really do not like when things that used to work "suddenly start to fail". It'd probably be against SemVer too.

For Jackson 3.0 we could and probably should move them. Maybe there should be a combo package of "jackson-datatype-jdk-extras" or something... included under jackson-datatypes-misc repo?

I'd need help with this, at least for verification, given that I still develop and work on Java 8 platform and have no strong knowledge or actual itch to scratch.

@cowtowncoder
Copy link
Member

Additional note: above was wrt actually getting rid of dependencies. Making import "weaker" might make sense for 2.x; I guess what I'd really like to know is just how to handle error cases. There is already dynamic loading for some of the features -- in fact, java.beans ((3) above) might already work "just fine" since these annotations are dynamically loaded via Java7SupportImpl (in 2.x).

And second part of course would be that of documenting common error cases.

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Nov 1, 2020

@GedMarc my use case is desktop, so smaller image is better for me
@cowtowncoder

On practical terms, I am not sure if anything can or should be done for Jackson 2.x; users really do not like when things that used to work "suddenly start to fail". It'd probably be against SemVer too.
Things will not fail if user does not use module system, since all modules are present;
and if module system is used, then things wouldn't faill too, since jackson does not require java.sql or other modules transitively (requires transitive module.name)

If jackson did requires transitive java.sql, then changing it to requires static or just requires would be incompatible change, since requires transitive java.sql exports java.sql to jackson users

java.logging (grep -rl "java.util.logging" .):

./jsontype/impl/SubTypeValidator.java // entry in list of strings, contains a lot classes from spring and other frameworks
./jsontype/DefaultBaseTypeLimitingValidator.java // entry in list of strings
./ext/beans/JavaBeansAnnotations.java // comment

java.desktop (package searched java.beans)

// INFER_CREATOR_FROM_CONSTRUCTOR_PROPERTIES(true)
// if user of jackson does not depend on `java.desktop`, this feature is pointless,
// since calling code can not pass `java.beans.ConstructorProperties` or `java.beans.Transient`
./MapperFeature.java

// implementation of MapperFeature
./ext/beans/JavaBeansAnnotationsImpl.java
./ext/beans/JavaBeansAnnotations.java
./introspect/JacksonAnnotationIntrospector.java 

// a lot of tests, docs and package-info.java ommited

java.sql, DateDeserializers static block needs little change

./deser/std/DateDeserializers.java
./ser/std/SqlDateSerializer.java
./ser/std/SqlTimeSerializer.java
./ser/std/StdJdkSerializers.java

I have not checked javax.xml usages, but i think situation is same as with java.sql

./ext/CoreXMLDeserializers.java
./ext/XMLGregorianCalendarSerializer.java
./ext/OptionalHandlerFactory.java
./ext/DOMDeserializer.java

Looks like most of the code is ready to be turned in optional.
I think it is safe to switch from requires to requires static with some minor code changes.

Since jackson does not require these modules transitively, calling code definitely can't pass classes from those modules without explicitly requiring those modules.

note: i did not heavily checked anything, i just did some grep and checked code in vim, it took like 15min except writing this comment, it took more

@GedMarc
Copy link

GedMarc commented Nov 1, 2020

@XakepSDK thought I would show you this - The base module structure for JDK 9 is this

image

The java.bean package resides in java.desktop, and is used widely by everything - you'll find that i used the bottom levels to provide access -

WRT Java 15 and records, the java.bean class already caters for the property reading of these classes, switching the BeanPropertySerializer for something else is a core change, @cowtowncoder will need to say for here (fluent syntax property reading)

To do this I believe the modules will need to be defined more strictly for Jackson, I understand the hibernate module is going away, but to do this for you (remove java.sql, not java.desktop), but the Jackson modules will most likely be required to split out to a finer grain, maybe V3?

./introspect/JacksonAnnotationIntrospector.java 

This is the core jackson class - i do not believe it will be changing any time soon

It is quick to check, but a lot longer to check backwards compatibility and effect on support platforms, also to build JLink as there are quite a few funnies - especially with the release of Jakarta Faces 3.0.0 and Jakarta JSON 2.0.0 and JAXB 3.0.0 which i need to make a PR for the jackson-json module now sigh -

@GedMarc
Copy link

GedMarc commented Nov 1, 2020

hmm maybe using lombok's & mapstruct's property reader which support fluent and chaining.. but that would add an external dependency

@GedMarc
Copy link

GedMarc commented Nov 1, 2020

Oh it's transitive because it's a base JDK library and used by all consumers of Jackson :) it's to make the modular pathway smaller, and not require consumers to define them as requirements :)
It makes the modular pathway much neater! And in native imaging, reduces module-info files a lot, this is important because of this bug

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8240567

This is extremely important to use transitive! There is a 64kb limit on the final generated module-info, i would not recommend remove the transitive clause

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Nov 1, 2020

This is the core jackson class - i do not believe it will be changing any time soon

It shouldn't be changed at all, this class just weakly uses JavaBeansAnnotations, that's why it's listed
In fact, it already weakly implements usage of class JavaBeansAnnotations, so if java.beans class is missing for some reasons, it wouldn't fail.
JacksonAnnotationIntrospector uses JavaBeansAnnotations
JavaBeansAnnotations weakly loads JavaBeansAnnotationsImpl
JavaBeansAnnotationsImpl implements annotation processing and directly uses java.beans classes
This is from JacksonAnnotationIntrospector

    // NOTE: To avoid mandatory Module dependency to "java.beans", support for 2
    // annotations is done dynamically.
    private static final JavaBeansAnnotations _javaBeansHelper;
    static {
        JavaBeansAnnotations x = null;
        try {
            x = JavaBeansAnnotations.instance();
        } catch (Throwable t) { }
        _javaBeansHelper = x;
    }

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Nov 1, 2020

I believe that:

  1. java.desktop, java.logging both can be safely switched to static right now
  2. java.xml needs small rework in ./ext/OptionalHandlerFactory.java
  3. java.sql needs very simple change in ./deser/std/DateDeserializers.java

@GedMarc
Copy link

GedMarc commented Nov 1, 2020

lets give it a try and see -

https://github.com/FasterXML/jackson-jdk11-compat-test

@GedMarc
Copy link

GedMarc commented Nov 1, 2020

there's a few updates for Jakarta compatibility now that they've settled on module names i need to do as well

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Nov 1, 2020

Bruh, this whole time i was checking 3.0 branch. Branch 2.12 doesn't look worse. ± same level of work required

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Nov 1, 2020

I will test everything tomorrow and probably prepare PR if everything goes well

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 2, 2020

@GedMarc Jackson 2.12 has support for Java 15 Records without using any of JDK facilities beyond Java 7.
I am also skeptical of usefulness of JDKs property detection simply because Jackson's model for finding aspects is well beyond what Java Beans do. So although some low-level reflection parts are obviously useful and used, I don't expect to find many usable pieces at higher levels.

@XakepSDK Sounds good -- it'd be great to get PR soon as I am about to push 2.12.0-rc2 out, and getting this change there would be very useful wrt testing. I am trying to avoid having to do any further rcs before actual 2.12.0 goes out.

@GedMarc
Copy link

GedMarc commented Nov 2, 2020

@XakepSDK - JAXB module references java.desktop, looks like I made a note in it - can you have a look?
https://github.com/FasterXML/jackson-modules-base/blob/6b7fea8b38d8f13208297d12edc6486ebc19f787/jaxb/src/moditect/module-info.java#L8

@cowtowncoder
Copy link
Member

I think that usage:

                    if (name == null || MARKER_FOR_DEFAULT.equals(name)) {
                        name = Introspector.decapitalize(refType.getSimpleName());
                    }

might be relatively simple to replace. And probably was earlier replaced from jackson-databind, although not sure if that implementation (from... BeanUtil, I think) is available (2.12 tries to remove static functionality in favor of pluggable implementation for property name mangling).

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Nov 2, 2020

I need some help, more info in PR

cowtowncoder pushed a commit to FasterXML/jackson-jaxrs-providers that referenced this issue Nov 10, 2020
@cowtowncoder cowtowncoder added 2.12 and removed to-evaluate Issue that has been received but not yet evaluated labels Nov 25, 2020
@cowtowncoder cowtowncoder added this to the 2.12.0 milestone Nov 25, 2020
@ryanthon
Copy link

ryanthon commented Sep 7, 2021

Sorry to bring up an old issue, but I'm not seeing the changes for this in the compiled jars of any 2.12 release, even though the source changes are in the branch. Was it reverted?

@cowtowncoder
Copy link
Member

@ryanthon I don't think so; I don't recall any reverts. 2.12.5 was released just recently.

@ryanthon
Copy link

ryanthon commented Sep 7, 2021

Maybe I'm not looking at the right thing, but doesn't the module-info of the jar need to state "requires static" for the modules described in the OP? I'm not seeing that when I inspect it in IntelliJ.

When I use jdeps to list my dependencies, I'm still seeing java.sql and java.desktop in the output.
Do these changes simply mean that I no longer need to include java.sql and java.desktop when using jlink if I don't need them? jackson-databind is the only dependency I have that uses them.

@cowtowncoder
Copy link
Member

@ryanthon Alas, those dependencies are needed due to oddities of JDK handling.

  • java.desktop is, I think, needed for java.beans package: for @ConstructorProperties and @Transient annotations
  • java.sql for couple of datatypes java.sql.Date / Time / Blob

These are quite tricky; support could be dropped/modularized away from databind in 3.0, but unlikely for 2.x.
In particular, I think @ConstructorProperties is widely used. Java SQL types is probably less of an issue, but even there it would be a breaking change for some users.

Other than that maybe @GedMarc can help here wrt "requires static".

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 a pull request may close this issue.

4 participants