Clojurians Log v2

cljs.user=> (max #js {:valueOf (fn [] (prn :value-of))} #js {:toString (fn [] (prn :to-string))} #js {:toString (fn [] (prn :to-string)) :valueOf (fn [] (prn :value-of))}
       #_=>      )
:value-of
:to-string
:to-string
:value-of
#js {:toString #object[toString], :valueOf #object[valueOf]}

max is only for numbers

Any reason (max 1 2 3) couldn't expand into Math.max(1,2,3) instead of:

cljs.user=> (str (fn [] (max 1 2 3)))
"function (){\nvar x__10606__auto__ = (function (){var x__10606__auto__ = (1);\nvar y__10607__auto__ = (2);\nreturn ((x__10606__auto__ > y__10607__auto__) ? x__10606__auto__ : y__10607__auto__);\n})();\nvar y__10607__auto__ = (3);\nreturn ((x__10606__auto__ > y__10607__auto__) ? x__10606__auto__ : y__10607__auto__);\n}"

Does it handle NaN like Clojure?

cljs.user=> (js/Math.max 1 js/NaN)
##NaN

note that cljs has odd behavior related to NaN and non-numbers, such as nil and (max "x" "y") (https://github.com/jank-lang/clojure-test-suite/blob/27bdda5969fe625d1498ee95f5ba28b64374694e/test/clojure/core_test/max.cljc)

that is testing the implementation, not the contract per docstring: max is only for numbers as @dnolen said above

a typical case of overtesting

given clojure and clojurescript's backwards compatibility goals, i think it's worth mentioning, even if i'm fine with changing the implementation

Note that I started this thread as questioning that, but the docstring is clear about it. Preserving backwards compatibility about something that is against the docstring is not something that Clojure/Script should have to limit itself to

i agree

(@dnolen if you would like an issue + patch for Math.max, let me know)

@dnolen quite a perf boost too:

cljs.user=> (simple-benchmark [] (max 1 2 3) 100000000)
[], (max 1 2 3), 100000000 runs, 4696 msecs
nil
cljs.user=> (simple-benchmark [] (max 1 2 3) 100000000)
[], (max 1 2 3), 100000000 runs, 4606 msecs
nil
cljs.user=> (simple-benchmark [] (js/Math.max 1 2 3) 100000000)
[], (js/Math.max 1 2 3), 100000000 runs, 43 msecs
nil
cljs.user=> (simple-benchmark [] (js/Math.max 1 2 3) 100000000)
[], (js/Math.max 1 2 3), 100000000 runs, 45 msecs

[f (fn* [p1__656# p2__657# p3__658#] (max p1__656# p2__657# p3__658#))], (f 1 2 3), 100000000 runs, 5114 msecs
nil
cljs.user=> (simple-benchmark [f #(js/Math.max %1 %2 %3)] (f 1 2 3) 100000000)
[f (fn* [p1__661# p2__662# p3__663#] (js/Math.max p1__661# p2__662# p3__663#))], (f 1 2 3), 100000000 runs, 408 msecs
nil

Probably can't benchmark this directly unfortunately, it will become a no op

ah I see, the recent CLJS has removed the macroexpansion logic to just call the runtime function:

cljs.user=> (str (fn [] (max 1 ##NaN)))
"function (){\nreturn cljs.core.max.call(null,(1),NaN);\n}"

I think this came up in the dialects meeting :)

it will be extremely hard to measure a call to Math.max, VMs can see that nothing is happening

easier to see if you swap it in the persistent data structure code

I was assuming the macroexpansion logic was still in there, hadn't seen this change yet (out of habit I use plk a lot)

if it does handle all cases and it is fast(er) there than unchecked-max|min - then that would be a obvious win

unhecked-max/min is basically the old version of max, right

yes

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/max

there are other repercussion here though, returns Infinity if no params

do you see others than that?

also it coerces to NaN

so different set of footguns than before

this needs more careful thought than may appear

agreed

this looks like a legit benchmark right?

cljs.user=> (let [obj #js {:res 0}] (simple-benchmark [] (set! (.-res obj) (+ (.-res obj) (js/Math.max 1 2 3))) 100000000) obj)
[], (set! (.-res obj) (+ (.-res obj) (js/Math.max 1 2 3))), 100000000 runs, 99 msecs
#js {:res 300000000}

cljs.user=> (let [obj #js {:res 0}] (simple-benchmark [] (set! (.-res obj) (+ (.-res obj) (max 1 2 3))) 100000000) obj)
[], (set! (.-res obj) (+ (.-res obj) (max 1 2 3))), 100000000 runs, 5030 msecs
#js {:res 300000000}

cljs.user=> (max)
WARNING: Wrong number of args (0) passed to cljs.core/max at line 1 
nil

I don't know what happens when you pass it non-numbers

Basically it would be good to know exactly what happens now and what will happen later.

defeating VM optimization is really hard

nothing but testing something completely non-trivial is going to tell us anything.

cljs.user=> (js/Math.max "a" "b")
##NaN
cljs.user=> (max "a" "b")
"a"

it doesn't matter, it all needs to be written down - changing stuff is super annoying even for undefined stuff. I'm not going to look at ticket w/o knowing exactly all the repercussions - especially if it's not particular high value.

sure

this way if we do - it we can enumerate all the changed expectations in changes.md, even if it was undefined.

that said, I am liking the general direction of the perf numbers - seems like this is a optimization path

probably similar but what happens to perf if you pass a string. I suspect no change - but what would be a good thing to see

in the old days weird stuff could trigger deoptimization in suprising ways

> probably similar but what happens to perf if you pass a string didn't catch this one. string is undefined for max, so what does perf matter?

just curious to see what happens, Math.max is not historically that good - one thing is that Math.max is about Double, but in many situations users may want to operate on 32bit integer.

they give different results so I think perf isn't relevant probably in that case? you get a different result slower or faster

it's not quite so simple as that - for example if bad code triggers deopt this could in theory be avoided w/ checks - at least before deopt could be contagious

I'm asking a bunch of questions because we never used Math.max therefore we don't have much information about what will happen. For example I know that the old (max (int a) (int b)) would generate optimal code, some comparisons on 32bit ints that's it

Math.max is a black box, so all these questions

my question was largely based on the assumption that the old macro stuff was still in place and it seemed to generate more boilerplate than necessary in comparison to Math.max. So I basically asked a bunch of questions to learn more. Why was Math.max never targeted, it existed since JS 1.0? Maybe there was a reason?

It was slow before

not a little bit slow, like really slow

gotcha

thus all my questions, it could be everything is resolved and that would be cool

it would break programs relying on the undefined behavior. although Rich stressed that "undefined" is really important to communicate and the docstring says nums, I don't know if I want to put in the effort of writing an issue+patch if breaking programs that rely on undefined behavior is a concern

that's a reasonable too, changing low level stuff is really a pain now.

Again that why I was suggesting less trivial test w/ the persistent data structures - is Math.max faster than unchecked-max in a real program

if there's some gold to mine - then we're not just looking at costs

I didn't get what you mean with "less trivial test w/ the persistent data structures" - can you explain?

every single op actually matters for PersistentVector for example.

everytime we removed a check, or collapsed a complicated test into logical operators we saw some gain

(int n) etc matter since that coerces to 32bit so the JS VM knows what to do

what's the relation between max and PersistentVector?

because we use it internally

But if Math.max does everything including the check plus being as fast or faster - then that's a win

as demonstrated in something the VM cannot handwave away

got it. so assoc on vector with index uses unchecked-max, so if this becomes lots faster, this will be relevant

yeap

and then we have some real value to offer to offset any undefined changes

how do I know I'm not looking at some no-oppy thing here?

cljs.user=> (simple-benchmark [a [1]] (assoc a 1 2) 100000000)
[a [1]], (assoc a 1 2), 100000000 runs, 3095 msecs

the VM won't no-op persistent data structure code it's just too complicated

with Math.max:

cljs.user=> (simple-benchmark [a [1]] (assoc a 1 2) 100000000)
[a [1]], (assoc a 1 2), 100000000 runs, 2356 msecs

whoa

well there you go 🙂

nice! patch welcome, it would be nice to enumerate the undefined behaior in the ticket

(oops, the index should have been 0, let me double check that)

yeah sorry that changes things. Math.max:

cljs.user=> (simple-benchmark [a [1]] (assoc a 0 2) 100000000)
[a [1]], (assoc a 0 2), 100000000 runs, 2454 msecs

cljs.user=> (simple-benchmark [a [1]] (assoc a 0 2) 100000000)
[a [1]], (assoc a 0 2), 100000000 runs, 2193 msecs

the other thing in the ticket would be to remove unchecked-min/max and just use this uniformly

10% is still great

it's slower

oh hah 🙂

right that's what I was worried about

you have to be careful w/ trivial benchmarks

now I get equal numbers roughly, so probably max didn't matter here much

I messed up the index, should've been 0

for assoc-in on [1]

oh

but in this case we want to go lower level

(-assoc ^cljs.core/IAssoc a ...)

👍

or whatever the protocol is for -assoc

Math.max:

cljs.user=> (simple-benchmark [a [1]] (-assoc ^IAssoc a 0 2) 1000000000)
[a [1]], (-assoc a 0 2), 1000000000 runs, 10468 msecs

cljs.user=> (simple-benchmark [a [1]] (-assoc ^IAssoc a 0 2) 1000000000)
[a [1]], (-assoc a 0 2), 1000000000 runs, 14062 msecs

so we do see quite a gain here

that's pretty good

so ticket great, it would be great to eliminate unchecked-min/max and use this instead uniformly

different VMs behave differently so it would be good to collect those other numbers in the ticket, I'm happy to add those to ticket later if you have a patch

is it possible with -re node to change the underlying VM, e.g. bun?

no people have asked for that, might even be a patch but would need to apply that

bun seems popular enough now so not against it.

then we can test v8 (node) and JSC (bun). others that are important these days?

Firefox, those are the 3

I suspect it'll be good, in the past we would avoid anything that slowed another engine down

I could also spit out an advanced compiled program and run it in a browser console

yeap

ok will write up the ticket+patch soon

hopefully tomorrow

It would be interesting to see how we compare to Clojure JVM after this change for fun - at one point I recall it being w/in 1.5X of JVM Clojure for PVec for some ops

user=> (time (let [a [1]] (dotimes [i 1000000000] (assoc a 0 2))))
"Elapsed time: 4027.172625 msecs"

(similar in 1.12.3)

to be clear I meant JSC

@dnolen I did benchmarks in chrome, firefox, node, bun but unfortunately I don't see any meaningful difference

My method was:

(ns entry)
(simple-benchmark [a [1]] (-assoc ^IAssoc a 0 2) 1000000000)

clj -M -m cljs.main --compile-opts '{:target :nodejs :optimizations :advanced}' -o out/main.js -c entry

having said this, max may still be slower than unchecked-max of course

and I noticed that unchecked-max with more than two args falls back on max

cljs.user=> (str (fn [] (unchecked-max 1 2 3)))
"function (){\nreturn Math.max((1),(2),(3));\n}"

There is a weird thing sometimes about advanced - try :simple with :static-fns true

oh and :optimize-constants true

it might not make a difference, but sometimes advanced compilation can slow things down a bit

that's weird. with your suggested options:

$ node out/main.js
[a [1]], (-assoc a 0 2), 1000000000 runs, 14911 msecs

$ clj -M -m cljs.main --compile-opts '{:target :nodejs #_#_:optimizations :simple #_#_:static-fns true #_#_:optimize-constants true}' -o out/main.js -c entry
$ node out/main.js
[a [1]], (-assoc a 0 2), 1000000000 runs, 2910 msecs

with only advanced:

node out/main.js
[a [1]], (-assoc a 0 2), 1000000000 runs, 3296 msecs

hrm that seems really weird, feel free to make a patch + issue anyway. It would be good to capture the work so far. Also, I can try to reproduce these results on my machine

ok

do you want to preserve "unchecked-min/max" after patch or can they go since they were only introduced last release?

I mean, rename back to min/max, the macros

doesn't matter, I'll preserve them

can rename them later if you want, small effort

hmm, it's better to rename unchecked-max to max so we get the benefit of turning (max 1 2) into a direct Math.max call

we can preserve the old name as well if you want

https://clojure.atlassian.net/browse/CLJS-3467

patch included

at least all core tests work

Don't need to preserve those names

thanks!

I didn't do benchmarks on new max vs old max. I suspect that will be faster. Now we only benchmarked old unchecked vs new