ClojureScript

Delegate clojure.string/capitalize to goog.string/capitalize

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 1.9.854
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Accepted

Description

Since the implementation of clojure.string/capitalize (6 years ago), Closure library added an identical function (3 years ago), which is actually much faster:

cljs.user=> (simple-benchmark [s "abc"] (goog.string/capitalize s) 10000000)
[s "abc"], (goog.string/capitalize s), 10000000 runs, 1181 msecs
nil
cljs.user=>  (simple-benchmark [s "abc"] (clojure.string/capitalize s) 10000000)
[s "abc"], (clojure.string/capitalize s), 10000000 runs, 5295 msecs

We could just have ClojureScript's delegate to Closure's.

  1. CLJS-2300.patch
    04/Aug/17 10:53 PM
    3 kB
    Mike Fikes
  2. CLJS-2300-2.patch
    01/Oct/17 9:34 AM
    3 kB
    Mike Fikes
  3. CLJS-2300-3.patch
    01/Oct/17 11:50 AM
    4 kB
    Mike Fikes

Activity

Hide
Mike Fikes added a comment -

Before/After tests look promising:

Before:

Benchmarking with V8
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 166 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 380 msecs
Benchmarking with SpiderMonkey
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 833 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 1541 msecs
Benchmarking with JavaScriptCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 53 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 438 msecs
Benchmarking with Nashorn
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 903 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 1181 msecs
Benchmarking with ChakraCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 345 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 727 msecs

After:

Benchmarking with V8
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 28 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 28 msecs
Benchmarking with SpiderMonkey
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 263 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 263 msecs
Benchmarking with JavaScriptCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 5 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 327 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 327 msecs
Benchmarking with ChakraCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 57 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 63 msecs
Show
Mike Fikes added a comment - Before/After tests look promising:
Before:

Benchmarking with V8
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 166 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 380 msecs
Benchmarking with SpiderMonkey
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 833 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 1541 msecs
Benchmarking with JavaScriptCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 53 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 438 msecs
Benchmarking with Nashorn
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 903 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 1181 msecs
Benchmarking with ChakraCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 345 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 727 msecs

After:

Benchmarking with V8
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 28 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 28 msecs
Benchmarking with SpiderMonkey
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 263 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 263 msecs
Benchmarking with JavaScriptCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 5 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 327 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 327 msecs
Benchmarking with ChakraCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 57 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 63 msecs
Hide
Mike Fikes added a comment -

The benchmarks produced in the previous comment are with included in the attached patch.

The patch also includes a generative regression test, using the previous implementation as a reference.

All engines pass the test fine if you crank up the number of values tested, except for ChakraCore. ChakraCore appears to be buggy, and I'll follow up upstream with a ticket for them. At its core, if you capitalize "ß" you should get back "SS". With the original ClojureScript implementation, you get back "ﮰ" on ChakraCore, and with the revision in the patch (essentially Closure Library) you get back "鯪", neither of which are correct. So, this patch doesn't introduce a regression on ChakraCore.

Show
Mike Fikes added a comment - The benchmarks produced in the previous comment are with included in the attached patch. The patch also includes a generative regression test, using the previous implementation as a reference. All engines pass the test fine if you crank up the number of values tested, except for ChakraCore. ChakraCore appears to be buggy, and I'll follow up upstream with a ticket for them. At its core, if you capitalize "ß" you should get back "SS". With the original ClojureScript implementation, you get back "ﮰ" on ChakraCore, and with the revision in the patch (essentially Closure Library) you get back "鯪", neither of which are correct. So, this patch doesn't introduce a regression on ChakraCore.
Hide
Mike Fikes added a comment -
Show
Mike Fikes added a comment - ChakraCore bug: https://github.com/Microsoft/ChakraCore/issues/421
Hide
Mike Fikes added a comment -

CLJS-2300-2.patch rebases against current master.

Show
Mike Fikes added a comment - CLJS-2300-2.patch rebases against current master.
Hide
Mike Fikes added a comment -

I took a look at ChakraCore's upper-case bug, and it is fairly non-trivial for them to fix. (The code relies on upper-casing strings in-place and thus can't handle things like ß -> SS .) Additionally, SpiderMonkey does the wrong thing mapping ß -> ß.

Given that engines are evidently prone to get this wrong, and we don't want to live with unit tests failing forever, attached CLJS-2300-3.patch essentially works around this by limiting the generative test to covering ASCII strings.

Show
Mike Fikes added a comment - I took a look at ChakraCore's upper-case bug, and it is fairly non-trivial for them to fix. (The code relies on upper-casing strings in-place and thus can't handle things like ß -> SS .) Additionally, SpiderMonkey does the wrong thing mapping ß -> ß. Given that engines are evidently prone to get this wrong, and we don't want to live with unit tests failing forever, attached CLJS-2300-3.patch essentially works around this by limiting the generative test to covering ASCII strings.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: