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

Remove reflection #51

Closed
wants to merge 1 commit into from
Closed

Remove reflection #51

wants to merge 1 commit into from

Conversation

alekcz
Copy link

@alekcz alekcz commented Sep 20, 2020

Change
Added type hints where the following are used:

  • java.util.Properties
  • java.lang.Math
  • java.security.SecureRandom
  • java.net.URLEncoder
  • java.net.URLDecoder
    No functionality has been edited

Effect
No more reflection type downstream, making encore graalvm friendly.

Why
The instances of reflection in encore are bubbling down into my lib.
I'm hoping to make it compatible with graalvm so I've added type hints to encore so that the reflection is gone.

fix reflection
@ptaoussanis
Copy link
Member

@alekcz Hi Alexander!

I haven't used Graal and am not too familiar with it - could you please give some extra context?:

  • Any idea why these would reflect under Graal?
  • Any pointers re: the simplest way I can reproduce these reflection warnings under Graal?

Thanks!

@alekcz
Copy link
Author

alekcz commented Sep 20, 2020

Caveat: I'm no Graal expert or Java expert for that matter. All of the below is from reading and experimentation

GraalVM context
When GraalVM compiles Clojure code to native binaries in doing the slow boot time of the JVM is eliminated, but so are some of the benefits. Reflection being one of the main benefits that one gives up. If the GraalVM can't determine data types during compilation, the compilation fails. 99% of cases adding type hints fixes this.

Another issue is that certain classes cannot be on the heap during compilation and should rather be initialised at runtime. SSL is a frequent offender. This was the reason for http-kit/http-kit#434.

In terms of detecting reflection (these are learnings by trial and error):

  • Using (set! *warn-on-reflection* 1) is more effective that the global config. I have no idea why.
  • Using cloverage unearths reflection in dependencies as well (That's how I found the http-kit one). It also seems to be more 'diligent' in identifying instances of reflection than leiningen
  • In some cases it'll report reflection on line 1. This usually occurs when the reflection is so deep in the dependencies it can't find the line in the source or the reflection is in an (:import) and not a (:require).
  • There are times when Graal compiles despite the reflection but I still don't know which "kinds" of reflection still allow compilation.

@alekcz
Copy link
Author

alekcz commented Sep 23, 2020

I learnt today that the magic words are lein check

Source: https://stackoverflow.com/questions/4474341/where-can-i-specify-warn-on-reflection-in-a-clj-file

@ptaoussanis
Copy link
Member

ptaoussanis commented Sep 24, 2020

Hi Alexander!

Just to confirm that we're on the same page here: you're only seeing reflection in encore on Graal, right?
Nothing in encore should be reflecting without Graal.

Edit to add: there's some boxed math, but no reflection.

@alekcz
Copy link
Author

alekcz commented Sep 24, 2020

@ptaoussanis yeah in Graal and only in with java.net.URLEncoder and java.net.URLDecoder. Added the other hints since I was in the code anyway.

@ptaoussanis
Copy link
Member

yeah in Graal and only in with java.net.URLEncoder and java.net.URLDecoder

Okay, will need to try get Graal setup on my end. I don't understand why these calls would reflect.
The return type of str is already type-hinted, so I'm not sure why it'd have a problem here- will need to dig a little to understand.

Added the other hints since I was in the code anyway.

So just to clarify: you didn't see the other forms reflecting, even on Graal?

@alekcz
Copy link
Author

alekcz commented Sep 24, 2020

@ptaoussanis no I didn't see the other forms reflecting in Graal. I saw them when I used lein cloverage.

The easiest way I found is to just include lein-cloverage "1.2.0" as plugin temporarily. It'll identify the instances correctly. (Note: it fails to instrument all the code properly).

The other approach is to write a main function that will touch all the parts of encore then compile with GraalVMs native-image creator.

@ptaoussanis
Copy link
Member

no I didn't see the other forms reflecting in Graal. I saw them when I used lein cloverage.

Okay, thanks for clarifying. I'm not familiar with lein cloverage or what it's trying to do- will need to try check that then as well.

I'm not sure why it would suggest doing something like ^Double (double x) - that makes no sense to me.

In any case, will try take a look when I get a chance!

Cheers

@alekcz
Copy link
Author

alekcz commented Sep 27, 2020

I'm closing this one. I've done absolutely nothing and the reflection has disappeared. And I can't get it to reappear. Thanks for all the effort and sorry for the trouble.

@alekcz alekcz closed this Sep 27, 2020
@ptaoussanis
Copy link
Member

No problem- thanks for following up!

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