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

Reflection warning when using destructuring sugar #72

Open
vvvvalvalval opened this issue Feb 16, 2019 · 2 comments
Open

Reflection warning when using destructuring sugar #72

vvvvalvalval opened this issue Feb 16, 2019 · 2 comments

Comments

@vvvvalvalval
Copy link

vvvvalvalval commented Feb 16, 2019

The problem

I'm observing some Reflection Warnings at the REPL when calling the s-de/fn macro, e.g

(do
  (set! *warn-on-reflection* true)

  (s-de/fn
    [(x ?y)]
    (+ x (or y 0))))
;Reflection warning, /Users/val/projects/.../data_sources.clj:48:3 - reference to field _1 can't be resolved.
;Reflection warning, /Users/val/projects/.../data_sources.clj:48:3 - reference to field _2 can't be resolved.
;Reflection warning, /Users/val/projects/.../data_sources.clj:48:3 - reference to field orNull can't be resolved.
=>
#object[linnaeus.imports.spark_scripts.data_sources$eval25833$fn__25834
        0x49b1f844
        "linnaeus.imports.spark_scripts.data_sources$eval25833$fn__25834@49b1f844"]

Why I think it's important

Method/field calls done via reflection have very bad performance, which tends to matter in the context of sparkling IMO - the destructuring code will tend to be called for each element to process.

Investigating the cause

Looking at the source code for sparkling.destructuring, I suspect this is because the TupleN class on which to call ._1, ._2 etc. can't be inferred statically (which is understandable in the context of Clojure).

Proposed solutions

For tuple destructuring

I suggest giving the caller the opportunity to provide a type-hint using meta-data on the list form, e.g

(s-de/fn
  [^{::s-de/type-hint Scala.Tuple2} (x ?y)]
  (+ x (or y 0)))

Or maybe the higher-level:

(s-de/fn
  [^{::s-de/tuple-arity 2} (x ?y)]
  (+ x (or y 0)))

From there, the macro could emit a type hint itself, or dispatch to a function that has a type hint etc.

This is not super readable, so we may complement this by:

  1. Having a single boolean flag and let the macro figure the Tuple arity from the length of the list:
(s-de/fn
  [^::s-de/type-hinted (x ?y)]
  (+ x (or y 0)))
  1. Having this type-hint on the argument vector itself, specifying that all nested tuples should be automatically type-hinted:
(s-de/fn
  ^::s-de/type-hinted [(x ?y (u w z))]
  (+ x (or y 0)))

For Optional Destructuring

Actually, this one can be inferred statically - all it takes is to have the emitted code use an Optional type hint, e.g by emitting a call to optional-or-nil.

Happy to provide a PR if needed! Keep up the great work.

@chrisbetz
Copy link
Contributor

Hi @vvvvalvalval Thanks for that input. Currently I'm really busy finishing up another (company) project, but I'll look into this.

Happy hacking,

Chris

@vvvvalvalval
Copy link
Author

Hi @chrisbetz , just following up with this. Let me know if I can help.

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

2 participants