Welcome! Please see the About page for a little more info on how this works.

0 votes
in ClojureScript by

Example

(str #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) ; => "42"

The problem in the fact that ClojureScript uses concatenation to convert values to strings and that doesn't work well with objects which have valueOf() method overriden.

Example in js:

`
var obj = {

toString: function() { return 'hello'; },
valueOf: function() { return 42; }

};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'
`

Potential solution might be to use String() function. Using toString() won't work as described in this issue: http://dev.clojure.org/jira/browse/CLJS-847

35 Answers

0 votes
by

Comment made by: dnolen

We've switched to {{goog.string.buildString}} in master https://github.com/clojure/clojurescript/commit/94eb8a960fef6aaca4ba44b251cefbfa04d0f6ac

0 votes
by

Comment made by: nbeloglazov

Yes, that works. Cool, thanks!

0 votes
by

Comment made by: thheller

Sorry for re-opening.

I was doing some profiling of my code and noticed a warning in the profiling output about cljs.core/str.

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

goog.string.buildString = function(var_args) { return Array.prototype.join.call(arguments, ''); };

Given that we don't ever call it with more than one argument it is probably not best implementation choice.

Maybe skip the call and inline it ala

`
(defn str
"With no args, returns the empty string. With one arg x, returns
x.toString(). (str nil) returns the empty string. With more than
one arg, returns the concatenation of the str values of the args."
([] "")
([x] (if (nil? x)

     ""
     (.join #js [x] "")))

([x & ys]

(loop [sb (StringBuffer. (str x)) more ys]
  (if more
    (recur (. sb  (append (str (first more)))) (next more))
    (.toString sb)))))

`

I didn't follow this issue but why are we not using .toString? The buildString/array approach seems kind of hackish?

I'm not too sure about the overall impact but since cljs.core/str showed up pretty high in my profile I think this should be investigated further.

0 votes
by

Comment made by: thheller

Before:

;;; str [], (str "1"), 1000000 runs, 254 msecs [], (str 1), 1000000 runs, 266 msecs [], (str nil), 1000000 runs, 80 msecs [], (str "1" "2" "3"), 1000000 runs, 753 msecs

After:

;;; str [], (str "1"), 1000000 runs, 82 msecs [], (str 1), 1000000 runs, 86 msecs [], (str nil), 1000000 runs, 79 msecs [], (str "1" "2" "3"), 1000000 runs, 242 msecs

But I only tested V8, probably needs some verification.

0 votes
by

Comment made by: thheller

`
(defn str
"With no args, returns the empty string. With one arg x, returns
x.toString(). (str nil) returns the empty string. With more than
one arg, returns the concatenation of the str values of the args."
([] "")
([x1]

 (.join #js [x1] ""))

([x1 x2]

 (.join #js [x1 x2] ""))

([x1 x2 x3]

 (.join #js [x1 x2 x3] ""))

([x1 x2 x3 x4]

 (.join #js [x1 x2 x3 x4] ""))

...)
`

Does perform even better.

;;; str [], (str "1"), 1000000 runs, 40 msecs [], (str 1), 1000000 runs, 43 msecs [], (str nil), 1000000 runs, 96 msecs [], (str "1" "2" "3"), 1000000 runs, 117 msecs

How many args should it inline?

0 votes
by

Comment made by: dnolen

I'd be OK with up to 4 then variadic.

0 votes
by

Comment made by: thheller

There is some weird interaction between the code generated by the cljs.core/str macro and function.

The macro generates

`
(str "hello" 1 "world" :yo nil)

yields

[cljs.core.str("hello"),cljs.core.str((1)),cljs.core.str("world"),cljs.core.str(new cljs.core.Keyword(null,"yo","yo",1207083126)),cljs.core.str(null)].join('');
`

Given that str with 1 arg will basically unroll to

[["hello"].join(""), ...]

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

0 votes
by
_Comment made by: favila_

bq. Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

Chrome complains about the variadic function dispatch code in the same way, see CLJS-916 plus patch.

bq. I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

The Closure compiler is not smart enough to remove the intermediate array, which is why I filed CLJS-801 (which this ticket rolled back). I don't think JITs can do it either.

I am beginning to wonder if we should ignore the Safari 6.0.5 problem in CLJS-847 that started all this string mess. To recap:

# CLJS-801 is accepted, which removes {{\[123, x].join('')}} in the str macro case in favor of {{''\+123\+(cljs.core/str$arity$1 x)}} style code, which the closure compiler can precompute. At this time, the one-arg cljs.core/str function (not macro) calls toString on its argument.
# CLJS-847 is filed. On Safari 6.0.5 at higher JIT levels, calling toString on some things (possibly only unboxed numbers? definitely not strings) throws a TypeError. This is unquestionably a bug in Safari. David fixes by making one-arg cljs.core/str function call js-str instead of toString. js-str uses string-concat {{''+x}}.
# However, this breaks for objects that define valueOf (issue in current ticket), because in js {{''+x}} is the same as {{''+x.valueOf().toString()}} not {{''+x.toString()}}.
# David considers using {{String()}} and variations but rejects because of performance hit.
# David rolls back CLJS-801 from the string-concat back to the array-join style to fix.
# Nikita and I point out that rolling back CLJS-801 only fixes the str *macro*, not the string *function*, which still uses {{js-str}} and hence string-concat.
# David fixes the str *function* to use goog.string.buildString, which has the behavior of array.join. Behavior is now correct even on Safari 6.0.5.
# Thomas points out that buildString uses arguments in a way unoptimizable by v8, and now the str function (not macro) has a performance regression. He suggests using {{\[].join()}} directly.

So, there's a lot of back and forth on this issue, and it's all because of a bug in Safari 6.0.5 which no one has been able to reproduce first-hand because Safari 6.0.5 is old and rare. For some perspective, Safari 6.0.x was only available on Lion and Mountain Lion between July 25,2012 and June 11,2013. Before July 25,2012 Lion used Safari 5.1.x and there was no Mountain Lion. On June 11, 2013, both Lion and Mountain Lion switched to Safari 6.1.x which does not suffer from the toString TypeError bug (I checked--I have an iMac with Lion on it).  The only machines on Safari 6.0.5 are (Mountain) Lion machines which used software updates until late 2012-early 2013 and then stopped. I can't imagine this is a large number of people.

It is theoretically possible for me to run Safari 6.0.x on my Lion machine to actually test this, but I can't find a way to downgrade from 6.1.x.

I think the options are:
# Use array.join() for all stringification and take the performance hit (which we should quantify). Include a comment that this is only for Safari 6.0.x (only confirmed second-hand on 6.0.4 and 6.0.5) for future generations, who are going to think this is weird.
# Use CLJS-801 and toString (status quo before CLJS-847), and ignore this problem for Safari 6.0.x.
# Use CLJS-801, but add a {{number?}} check (with comment) to {{cljs.core/str$arity$1}} for Safari 6.0.5. The number case should use js-str, and the rest toString. I think this will work, but again we have no way to test--we really need to get our hands on a Safari 6.0.x browser.

Of course we should benchmark these approaches but my hunch is that 2 is faster than 3 is faster than 1.
0 votes
by

Comment made by: dnolen

We are not going to ignore Safari 6.0.X. Any decisions made about this ticket will include supporting it.

0 votes
by
_Comment made by: favila_

Update on some research I am doing into this.

I created a [jsperf of alternative str implementations|http://jsperf.com/cljs-core-str-possibilities] that I am trying out. Right now I've only looked at one-arg str. I discovered a few things:

* {{''+[x]}} is a faster alternative to {{\[x].join('')}}.
* Advanced compilation can compute {{''+[x]}} at compile time if {{x}} is a bool, str, undefined, null, or number, even through function calls! I.e. str_test.str_arr(123) compiles to "123" without macro magic.
* However, using an intermediate array (even if a preallocated singleton) is still slower than the old {{\(if (nil? x) "" (.toString x))}}
* Using a switch statement is as least as fast as the str-tostr baseline, usually faster.
* I am 99% sure all these implementations (except str-tostr, the baseline, which definitely fails) work on the dreaded Safari 6.0.x. If anyone has this version, point it at the jsperf link above and run the tests. I think [Browserstack|http://www.browserstack.com] has this version of Safari.

I'm still investigating the variadic case (str x y z a b c). It might be better to use reduce instead of Stringbuffer+seq. (Stringbuffer just does {{''+x}} now instead of an array-join.)
0 votes
by

Comment made by: thheller

Sorry, got side-tracked for a bit.

@Francis: Thanks for the recap.

Don't have Safari 6 available either, but seems wrong that we all have to suffer because is minor percentage still has this (667 users of 190k+ on my site). Don't have a solution cause I can't test whether it works, we might try String.concat.

"".concat(obj); // "42" "".concat(obj, ""); // "hello" String.prototype.concat(obj, "") // "hello" String.prototype.concat("", obj) // "hello"

But no idea if String.concat works, also it behaves odd with regards to valueOf.

http://jsperf.com/js-string-concat-variants

Perf is also inconclusive since Firefox appears to be cheating.

0 votes
by

Comment made by: favila

Tested that jsperf with Safari 6.0.5 using Browserstack, results are there.

Note I could not reproduce CLJS-847 because str-tostr does not fail as expected. I will try harder now that I have a browser to test.

0 votes
by

Comment made by: favila

Still cannot reproduce CLJS-847.

(link: https://dl.dropboxusercontent.com/u/18386249/code/strtest.html text: This script) includes my attempt at a minimum reproducible case. My theory was that certain types at higher jit levels would fail. I could not get any case to fail. I also tried flapping back and forth between types and using one type at a time, but still no failures.

In (link: https://github.com/wycats/handlebars.js/pull/440 text: this thread) I found (link: https://dl.dropboxusercontent.com/u/18386249/code/handlebar_str_typeerror.html text: this "minimal" script) which the OP said he could get to fail reliably. I could not get it to fail. However the original post was from feb 15, 2013, which means the Safari he was using would have to be 6.0.2 or lower.

Hypotheses:

  1. This error does not affect 6.0.5 but maybe 6.0.4 or lower.
  2. BrowserStack's system somehow mitigates the bug, meaning we need a "real" Lion Safari 6.0.x to test.
  3. These tests only fail under the correct phase of the moon.

So I can code up a patch for str using the str-switch implementation (which is at least a bit faster on some browsers), but I have no idea if it may fail on Safari 6.0.5. I only know that it works so far. CLJS-801 should also be safe to reapply because the root cause of all issues is the implementation 1-arity of the cljs.core/str function.

I have also asked for Kevin's help back in CLJS-847. (Kevin was the original reporter of the Safari 6.0.x issue.)

0 votes
by

Comment made by: favila

Made a (link: http://jsperf.com/cljs-core-str-variadic-possibilities text: jsperf of variadic cases). Chrome seems to really prefer IReduce to seq+stringbuilder for vectors (other collections not tested), but there is no difference or a small slowdown on other browsers. Not sure if it's worth it.

Also updated (link: http://jsperf.com/cljs-core-str-possibilities/2 text: arity-one cases) with a str using switch and never using toString. Nearly 50% slower than using switch or toString on Chrome, smaller on Safari.

In terms of safety str-switch-notostr does not use toString at all so is probably safer. I think str-switch will likely work too, though, and is significantly faster. However I haven't been able to get any TypeErrors in Safari 6.0.5 so it's anyone's guess.

I suggest something like this as a new str (which doesn't use reduce, but could):

`
(defn str
([x]
(case (js "typeof ~{}" x)
"string" x
"object" (if (identical? nil x) "" (.toString x))
("boolean" "number") (js-str x)
"undefined" ""
(js-str #js [x]))) ;; insurance against Safari 6.0.x TypeError bug.
([a b] (js
"~{}+~{}" (str a) (str b)))
([a b c] (js "~{}+~{}+~{}" (str a) (str b) (str c)))
([a b c d] (js
"~{}+~{}+~{}+~{}" (str a) (str b) (str c) (str d)))
([a b c d & more]
(loop [s (str a b c d) [e f g h & r] more]
(let [s' (js* "~{}+~{}+~{}+~{}+~{}" s e f g h)]

(if (nil? r)
 s'
 (recur s' r))))))

`

0 votes
by

Comment made by: favila

First cut of a possible patch that resolves this while not breaking CLJS-847. Should wait for confirmation that this does not break Safari 6.0.x.

...