@dnolen @borkdude and I just figured out that (Array/.push #js [] 1) generates suboptimal code, since it does generate a Reflect wrapper, the thing this is supposed to prevent in CLJ. though it might be a bad implementation in shadow-cljs on my part, but appears to be in CLJS as well. see
cljs.user=> (str (fn [] (js/console.log (Array/.push #js [] 1))))
"function (){\nreturn console.log((function (){var G__15508 = [];\nvar G__15509 = (1);\nreturn (function (x, ...args) { return Reflect.apply(Array.prototype.push, x, args) }).call(null,G__15508,G__15509);\n})());\n}"
whereas the alternative without the Array/ bit just generates (.push ...) as expected.@thheller you can read dnolen's take on this here already in some extend: https://clojurians.slack.com/archives/C07UQ678E/p1764338506275709 I think he's of the opinion that Reflect doesn't add any significant overhead
but your example does show it bloats the generated code
it is still the exact overhead that the CLJ variant is suppose to prevent, so exact opposite ergonomics when compared. so IMHO that should absolutely be addressed, otherwise its a huge footgun for anyone coming from CLJ (or writing portable code)
also fairly simple to address
I think that's a fair point: By using method values in CLJ you will prevent reflection so you'll get better perf and codesize, whereas in CLJS it's the opposite
Reflection in JS is nothing like Reflection in Java, the overhead is largely uninteresting in my opinion.
But, if someone wants to remove the tiny overhead, I'm ok w/ that!
Just not interested in spending energy here myself at the moment.
the code size thing okay, but also seems minor. I agree w/ the earlier points that unless you're in a situation where you're saving a few characters or dealing w/ code sharing, I don't see why you would use this form.
trying to understand what you mean with "don't see why you would use this form", what form?
Array/.push - which already a weird case since I can't imagine doing this.
for context: https://clojurians.slack.com/archives/C6N245JGG/p1764580552328909?thread_ts=1764240551.710409&cid=C6N245JGG
someone was trying to write a macro that looked similar in the clj and cljs branches
so they wrote something like:
:clj ArrayList/.add
:cljs Array/.push
and they didn't know about the trade-offsthey also encountered an issue with Array being unresolved in namespace where they used this macro (which is expected I think but one thing that may surprise people with :refer-global and macros)
While I understand the confusion, I don't see any real surprises here, just normal resolution rules.
ClojureScript doesn't have the exponential code gen issue in that thread because that's handled uniformly.
Re: “I don’t see why you would use this…“, I can give you one sample from a user’s point of view: I particularly like the self-documenting aspect of method values, thus far in Clojure.
(-> v
(.push)
(.pull)
(.explode))
(-> v
(Hello/.push)
(World/.pull)
(Surprise/.explode))
When working in an interop heavy NS, I find it that it helps understandability to be clear that eg. pull returns a Surprise and not something else.
That this would lead to performance tradeoffs is surprising as compared to Clojure.
Either way, it’s OK, I just won’t use it, as per recommendation.FWIW I "fixed" this via https://github.com/thheller/shadow-cljs/commit/da87191e2db83c3cb34415cdef19d479e22a1795. at least I think so, still need some more testing to see if thats actually correct.
There is no recommendation to not use it from me at least. Again I am not concerned at all in its current form. In actually higher order usage the performance is going to be dominated by other things. These benchmarks so far are very naive IMO
I don't care much about performance in this case either. I just see it as "incorrect" behaviour. In CLJ you do this to prevent reflection. In CLJS you do this to cause "reflection". Better performance and less code is just a side effect.
but .. there is a case were you actually want to reflect, so makes sense either way. Calling a specific implementation in case you want to bypass an override somewhere. could still opt to Array.prototype.push.call for that case, just to bypass args/apply. not sure how relevant this is either way. still haven't looked into this one bit how CLJ does this.
I think that method values are primarily about using Java stuff as values, the details of how it looks is mostly about avoiding reflection to accomplish it (but that's not the primary goal), and ^:param-tags is necessary for ambiguous cases. As we said before, JS is already value oriented so it's not really a win besides making it easy to write some portable things in some situations. Using Reflect here solves the problem of generating correct code in the sense that we don't have to know anything about the ctors / prototypes / # arguments.
I agree that for Array etc. we have all the information to get rid of Reflect (everything in the processed externs), but this is polish vs. covering all cases w/ Reflect as baseline
yeah, but the whole point is that in JS we do not need to know any extra information. we can just call things and let the runtime sort it out. so why should supplying extra information (i.e. Array/) make it "worse", meaning less performant, more code generated. I'm not arguing at all about the standalone method value case. I'm specifically only talking about the direct invoke case just as (Array/.push ...), where we know the maximum amount of information (e.g. number of args). So, at the very least this could drop the extra generated IIFE and still use Reflect.apply. or it could just call things, IMHO makes sense to mirror the CLJ behavior which I have not checked.
If the recommendation becomes that in CLJ do this and in CLJS do the opposite, then thats bad.
Oh ok, only direct invoke case. That does seem reasonable. Need to think about the right way to handle that. I didn’t examine that part of what CLJ does - I did look at it quite a bit for inspiration but not that part.
the shadow tweak above where you check in invoke parsing seems ok, need to think about that some more
hrm did tools.analyzer.jvm do anything for method values @bronsa?
There are jiras for it and I think Fogus did at least some of the work
not yet unfortunately. it's on my todo list but i've been crazy busy for the last year+ so haven't gotten to it yet