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

Is kotlin-reflect really needed for lets-plot? #471

Closed
vlsi opened this issue Nov 2, 2021 · 6 comments
Closed

Is kotlin-reflect really needed for lets-plot? #471

vlsi opened this issue Nov 2, 2021 · 6 comments

Comments

@vlsi
Copy link

vlsi commented Nov 2, 2021

I'm about to add lets-plot charts to Apache JMeter, and it turns out kotlin-reflect-1.5.21.jar consumes 3 megabytes (~18% of the new jars).

Could kotlin-reflect dependency be avoided in lets-plot?

Just for reference, here's the list of all the jars Kotlin + lets-plot brings:

    54852334 => 71166807 bytes (+16314473 bytes)
    99 => 134 files (+35)

  +   17536 annotations-13.0.jar
  -   18538 annotations-16.0.2.jar
  +  507197 base-portable-jvm-2.1.0.jar
  +  485809 batik-anim-1.14.jar
  +  424624 batik-awt-util-1.14.jar
  +  703757 batik-bridge-1.14.jar
  +  112373 batik-codec-1.14.jar
  +    8433 batik-constants-1.14.jar
  +  330318 batik-css-1.14.jar
  +  184487 batik-dom-1.14.jar
  +   10238 batik-ext-1.14.jar
  +  192087 batik-gvt-1.14.jar
  +   11466 batik-i18n-1.14.jar
  +   76875 batik-parser-1.14.jar
  +   25876 batik-script-1.14.jar
  +    6663 batik-shared-resources-1.14.jar
  +  232734 batik-svg-dom-1.14.jar
  +  227514 batik-svggen-1.14.jar
  +  129300 batik-transcoder-1.14.jar
  +  127477 batik-util-1.14.jar
  +   33866 batik-xml-1.14.jar
  +   32033 kotlin-logging-jvm-2.0.5.jar
  + 2993765 kotlin-reflect-1.5.21.jar
  + 1505952 kotlin-stdlib-1.5.31.jar
  +  198322 kotlin-stdlib-common-1.5.31.jar
  +   22986 kotlin-stdlib-jdk7-1.5.31.jar
  +   16121 kotlin-stdlib-jdk8-1.5.31.jar
  +  792176 kotlinx-html-jvm-0.7.3.jar
  +  196707 lets-plot-batik-2.1.0.jar
  + 3593892 lets-plot-common-2.1.0.jar
  +  627895 plot-api-jvm-3.0.2.jar
  +  882509 plot-base-portable-jvm-2.1.0.jar
  +  792534 plot-builder-portable-jvm-2.1.0.jar
  +  115007 plot-common-portable-jvm-2.1.0.jar
  +  454864 plot-config-portable-jvm-2.1.0.jar
  +  173932 vis-svg-portable-jvm-2.1.0.jar
  +   85686 xml-apis-ext-1.3.04.jar
@alshan
Copy link
Collaborator

alshan commented Nov 2, 2021

Hi, I have to double check on this but this is most likely a required dependency.

@vlsi
Copy link
Author

vlsi commented Nov 6, 2021

I'm not sure what are the build/test instructions (are you interested in GitHub Actions-based CI?), however, I commented all kotlin-reflect dependencies, and gradlew build more-or-less works for me.

There are several reflection usages, so I kept kotlin-reflect in plot-base-portable / jvmTest:

AesReflectTest which uses reflection to instantiate Aes:

val companion = Aes::class.companionObject!!
val properties = companion.declaredMemberProperties

LiveMapTestBase which uses .constructors..:
entity.addComponents{ + aType.constructors.first().call() }

BUILD SUCCESSFUL in 1m 58s
588 actionable tasks: 159 executed, 429 up-to-date

(I excluded python-extensions)

Other than that, it looks like kotlin-reflect is not really used in the implementation of lets-plot.

@vlsi
Copy link
Author

vlsi commented Nov 6, 2021

AFAIK Kotlin compiler is able to compile simple things like ::class.name and delegated properties without dependency on kotlin-reflect.

@vlsi
Copy link
Author

vlsi commented Nov 6, 2021

Just in case, the use of lets-plot in JMeter is very minimal now (~one line chart), and it works when kotlin-reflect dependency is excluded.

@alshan
Copy link
Collaborator

alshan commented Nov 7, 2021

Sounds good! If ::class.name indeed doesn't fail at runtime without kotlin-reflect then I don't see a reason to bundle this dependency.
For now it should be safe to just exclude it as you did.

gradlew build more-or-less works for me.
Yes, this is what we do.

@IKupriyanov-HORIS
Copy link
Collaborator

Yes, most reflection we are using is provided by stdlib (Klass.name, KProperty.name). Exceptions are AesReflectTest and LiveMapTestBase. In fact we already refactored LiveMap tests so there is no need in that kind of reflection, so I removed the code that depends on kotlin-reflect.

So what I did - I removed all kotlin-reflection deps except one - for jvmTest in plot-base-portable. I did excessive testing by running our JVM, Native and JS applications - everything looks fine. If you will notice any problems after this change, please, make us know.

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

No branches or pull requests

3 participants