Fork me on GitHub
#cljs-dev
<
2017-04-04
>
mfikes00:04:06

I attached the patch I've been trying to an "Outward function type hint propagation" JIRA: http://dev.clojure.org/jira/browse/CLJS-1997

anmonteiro00:04:27

interesting how small the patch is 🙂

mfikes00:04:33

Yes—it could be even smaller (but a little more aggressive) if it just dropped the inferred type directly into :ret-tag. But, yes—I was surprised the compiler is already arranged to support it.

mfikes00:04:45

The patch not only has potential perf benefits (eliding the truth check), but it could help catch more errors via type propagation:

cljs.user=> (defn f [] "h")
#'cljs.user/f
cljs.user=> (+ (f) 1)
            ^
WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 1
"h1"

rauh10:04:05

So it seems that Array.prototype.join already does work properly with nil + js/undefined:

(js* "[~{},~{},~{}].join('')" nil "foo" js/undefined)

rauh10:04:39

I'd like to propose to change the str macro to:

(let [pre-cat (reduce
                      (fn [xs x]
                        ;; If this and the last is a string, we can pre-concat:
                        (if (and (string? x) (string? (last xs)))
                          (conj (pop xs) (str (last xs) x))
                          (conj xs x)))
                      []
                      args)]
        ;; If all that's left is a single string we can just emit that:
        (if (and (= 1 (count pre-cat)) (string? (first pre-cat)))
          (first pre-cat)
          (apply core/list 'js/goog.string.buildString pre-cat)))

rauh10:04:11

That'd also allow people to just use (str "foo" "bar") to break up long string over multiple lines.

rauh10:04:33

And use goog.string.buildString (which just calls the Array.prototype.join)

rauh10:04:14

Similar for str function. There is

goog.string.makeSafe = function(obj) {
  return obj == null ? '' : String(obj);
};

thheller11:04:15

@rauh there is a lot of history behind the str macro, see http://dev.clojure.org/jira/browse/CLJS-890

thheller11:04:44

and FWIW the closure compiler will do the optimization you proposed (if I understand the intent correctly)

rauh11:04:02

Oh boy, ok going through this ticket.

rauh11:04:30

I don't see google closure compiler pre-concatenating strings unless I use js's +-operator

thheller11:04:11

I did this recently http://dev.clojure.org/jira/browse/CLJS-1879 which let the closure compiler inline more str related things

thheller11:04:01

it should inline ["foo", null, undefined].join("")] to just "foo"

rauh11:04:29

Yeah I so that ticket. But strange that the current impl is faster than buildString. Their docstring says the opposite.

thheller11:04:30

I did only this minimal change because I was afraid of the history of CLJS-890, but it provided most of the benefits

thheller11:04:26

but FWIW that ticket is also ancient history and probably not relevant today

rauh11:04:09

Ok I see, lots of edge cases 😕. Though: 1. The calls to the 1-arity version don't get optimized away even when passing a string literal 2. Why again .join("") call to concat the resulting strings when we already know that we have only strings. (So + should be safe to use, even for Safari)

rauh11:04:55

Ie, why not:

(core/let [strs (core/->> (mapv (fn [x]
                                        (if (string? x)
                                          "~{}"
                                          "cljs.core.str.cljs$core$IFn$_invoke$arity$1(~{})"))
                                      xs)
                                (interpose "+")
                                (apply core/str))]
        (apply list 'js* (core/str "(" strs ")") xs))

rauh11:04:53

GCC should concat the constants for us and we don't emit unnecessary calls to the arity$1 str function.

thheller12:04:09

so if I understand correctly you want (str "foo" "bar") to emit ["foo", "bar"].join("") instead of [cljs.core.str.cljs$core$IFn$_invoke$arity$1("foo"), cljs.core.str.cljs$core$IFn$_invoke$arity$1("bar")].join("")

thheller12:04:36

I'm fairly sure that closure will do that

rauh12:04:54

No, note the +, I'm using + concat after we have guaranteed that we deal with strings

rauh12:04:08

Nope, GCC def won't optimize the function calls away. At least not for me

rauh12:04:01

eg the proposed would result in: (cljs.core.str.cljs$core$IFn$_invoke$arity$1(ns) + "/" + cljs.core.str.cljs$core$IFn$_invoke$arity$1(name))

rauh12:04:42

The link doesn't work

thheller12:04:53

function str(x) {
  return x;
}
console.log([str("foo"), str("bar")].join(""));

thheller12:04:02

produces console.log("foobar");

thheller12:04:20

it optimized the functions calls and the join

rauh12:04:41

Try:

function str(x) {
if((x == null)){
return "";
} else {
return [x].join("");
}
}
console.log([str("foo"), str("bar")].join(""));

thheller12:04:39

ah the nil check

rauh12:04:29

See the pseudo advanced compilation. It's littered with cljs.core.str.cljs$core$IFn$_invoke$arity$1("some string")

thheller12:04:51

that should really be optimized by the macro then, I was sure closure took care of that

rauh12:04:43

Yeah it should also be faster, since after the call we can do fast + concatenation.

thheller12:04:23

I would stick to [].join("") it has basically the same performance

thheller12:04:29

but avoids the valueOf history

rauh12:04:16

But we don't need the join anymore, they're all guaranteed to be strings.

rauh12:04:42

Well so silly question: Why do we need the nil check?

rauh12:04:49

join will take care of that.

rauh12:04:57

And then it'll also be optimized away:

rauh12:04:10

function str(x) {
  return [x].join("");
}
console.log([str("foo"), str("bar")].join(""));

rauh12:04:30

Works with nil and with old Safari.

thheller12:04:32

some JS engine printed [null].join("") as "null"

favila14:04:24

I still think we should stop worrying about Safari 6.0.5. Even more so now since 2+ years have passed

favila14:04:12

That safari is only on not-fully-updated OS X Mountain Lion

favila14:04:29

I have an ancient mountain lion mac and even it uses safari 6.1.x, which does not have that str bug

rauh14:04:12

Agreed. This also indicates it can be safely dropped: http://caniuse.com/usage-table

dnolen16:04:56

@favila I don’t think that seems wise given iOS

dnolen16:04:12

changing the behavior of str is way low priority

favila16:04:09

Isn't that still, like iOS 6?

favila16:04:44

The only report we had was a desktop safari from a logged js stacktrace

rauh16:04:07

@dnolen I don't propose changing the behavior. The macro change would result in smaller & faster code. So IMO a win win without any downside.

dnolen16:04:22

I just can’t see how this will matter

dnolen16:04:24

did you benchmark?

dnolen16:04:39

like across at least 3 engines?

dnolen16:04:27

@favila that doesn’t really tell us anything - we fixed it for Safari so we don’t get reports about older iOS

rauh16:04:14

@dnolen I just tested it on ff+chrome. It looks like chrome prefers [].join(). So I'd say stick with array.join but I'd still vote for omitting the str$arity$1 call if it's already a string.

dnolen16:04:44

again it doesn’t seem important to me

favila16:04:46

@dnolen When I wrote CLJS-801 two years ago it was a dramatic difference, but the macro was a bit dumber then. (You also accepted it without comment)

dnolen16:04:00

churn where there is no measurable value isn’t something to spend time on

favila16:04:39

js engines may be better now, and emiting a direct str arity-1 call is an improvement

favila16:04:05

my goal then was to give closure more optimization chances