Fork me on GitHub
#cljs-dev
<
2017-05-30
>
rauh06:05:30

@shaunlebron I don't think you'd need the type hint in this case. I've been running with macro like this and it's working well:

(core/defmacro =
  ([x] true)
  ([x y]
   (core/cond
     (core/or (core/keyword? x) (core/keyword? y))
     (bool-expr
       (core/list 'cljs.core/keyword-identical? x y))
     (core/or (core/string? x) (core/string? y) (core/char? x) (core/char? y))
     (bool-expr
       (core/list 'cljs.core/identical? x y))
     (core/or (core/number? x) (core/number? y))
     (bool-expr
       (core/list 'cljs.core/== x y))
     :else
     (bool-expr
       (core/list 'js* "cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(~{},~{})" x y))))
  ([x y & xs]
   (bool-expr
     (core/list 'js*
                "cljs.core._EQ_.cljs$core$IFn$_invoke$arity$variadic(~{}, ~{}, ~{})"
                x y `(cljs.core/array-seq (cljs.core/array ~@xs) 0)))))

(core/defmacro not=
  ([x] false)
  ([x y] `(coercive-not (= ~x ~y)))
  ([x y & xs]
   `(coercive-not (= ~x ~y ~@xs))))

shaunlebron12:05:37

@rauh thanks! so it just sees if any arg is a constant to optimally select the appropriate comparison function, and falls back to original function

shaunlebron12:05:56

is this doable as a core patch?

mfikes13:05:09

An experiment to see if filter can be made to run faster if the predicate is a Var tagged ^boolean (using unchecked if): https://gist.github.com/mfikes/7a725048ce6eb92b6965393e913558b1

dnolen14:05:20

@mfikes interesting though the perf benefit is surprisingly small

mfikes14:05:51

Yep. Probably not worth pursuing further. Just sharing.

dnolen14:05:52

@rauh that version of = is probably not a good idea, you really can’t swap in keyword-identical? like that w/o introducing a breaking change and a lot of confusion

dnolen14:05:56

actually, sorry I was confused - was thinking about somebody having proposed this for identical?

dnolen14:05:25

@shaunlebron inlining = like @rauh has done is not a bad idea and would cover the Closure define case, patch welcome

dnolen14:05:39

perhaps my only suggestion would be to cover a few more cases like nil, true, false

dnolen14:05:21

hrm on a second thought doing this means breaking = extension yourself for these values

rauh14:05:20

We could straight up call IEquiv if one of them is a coll? like (= [0] the_exp) -> (.cljs$core$IEquiv$_equiv$arity$2 [0] nil the_exp) (possibly adding ^not-native)

rauh14:05:17

Also could do symbol-identical?. Though I think most common cases are covered by the above code.

dnolen14:05:35

@rauh what I mean is that inlining prevents would-be extenders

dnolen14:05:57

= is intentionally extensible

rauh14:05:24

Right, that's impossible to do then... And those extender folks would also rely on the fact that = function right now calls (-equiv x y), so IEquiv on the first argument (and not on the second argument). So we could change the above by only checking the first argument x for keyword?/`number?` (etc). and emit specialized code.

rauh14:05:55

But not if y is a keyword, since the custom type could potentially define its own IEquiv.

rauh14:05:03

So the question is if we wanted to optimize that then... Or just leave it as it is.

dnolen14:05:52

leave as is since it would be a big breaking change and has no effect on existing highly optimized code

shaunlebron19:05:21

so the breaking case is that someone overrides -equiv for keywords/booleans/strings?

(extend-type Keyword IEquiv (-equiv [o other] false))

shaunlebron19:05:26

worth mentioning that this type of overriding is ignored when comparing constants when optimize-constants is true, since = will first check identical?

shaunlebron19:05:13

perhaps that means we can still safely perform this optimization when comparing a goog-define to a constant (which is the motivating use-case here)

shaunlebron19:05:45

(i.e. a goog-define is limited to a string/number/boolean, so comparison to a constant would have the same current behavior as = due to the identical? shortcut)

dnolen19:05:36

@shaunlebron it’s not ignored

dnolen19:05:50

if identical? is false then we dispatch

shaunlebron19:05:01

hmm, if we have access to the goog-define constant at compile-time, we can still expand it to identical and ignore the dispatch case since we know they are equivalent

dnolen19:05:03

I think we should table any notion of messing around with base semantics of =

dnolen19:05:20

@shaunlebron yes, something less invasive like that is preferred

dnolen19:05:56

just special case goog-define vars - you’ll have declared their values for :advanced so we’ll know

shaunlebron19:05:32

k thanks, i’ll try patching