clojure.string/trim uses different defn of whitespace as triml, trimr
Description
Environment
Attachments
Activity
Rich Hickey January 3, 2014 at 3:28 PM
Is there a gist or anything of the perf tests?
Alex Miller December 3, 2013 at 4:55 AM
Testing perf of ltrim on a 1M character blank string (tested on criterium w/ good JVM settings):
clojure.string/ltrim = 2.785597 ms
clj935-2.patch ltrim = 2.880528 ms
same as clj935-2 with unchecked-inc = 2.225098 ms
same as clj935-2 with unchecked-inc-int = 2.198700 ms
I tested the last 3 with unchecked-math both true and false and saw no difference between them.
Based on this, I will update the patch to use unchecked-inc as that seems to give even better performance than the prior impl.
Rich Hickey October 25, 2013 at 1:42 PM
did you try
?
Alex Miller August 30, 2013 at 9:47 PM
In response to the questions regarding the "int" calls in the prior patch, I did some analysis and looked at alternative impls.
Prior patch: the calls to (int) were indeed causing primitive ints to be used in some cases (like letting the length). However, the loop index/rindex is always a prim long at best (and there is no way to type hint or alter that afaict). Because of this, casts get inserted to upcast the length to a long for comparison anyways. Using criterium and calling ltrim on a string of 1M spaces, I got a mean of 2.9 ms.
I tried a dirty trick of using a mutable int array for the index and that (while ugly) was 2.25 ms, so definitely faster. It did not seem fast enough to justify the dirtiness.
I also benchmarked a pure Java int version:
This code is actually smaller and faster - 0.92 ms.
I pondered this and in the end decided to leave it as the most straightforward Clojure impl. I think for the vast majority of trim calls, the difference will be negligible. For users needing higher trim performance, they should probably write an optimized version and tune their whitespace set. My second choice would be adding the Java implementations.
Marking screened.
Aaron Bedra April 11, 2013 at 11:34 PM
Bump on the discussion. This ticket seems blocked until we make a decision.
Details
Details
Assignee
Reporter
Andy FingerhutLabels
Approval
Patch
Priority
Minor
clojure.string/triml and trimr use Character/isWhitespace to determine whether a character is whitespace, but trim uses some other definition of white space character. For example:
Cause:
trimlandtrimruse Character/isWhitespace.trimuses String/trim which seems to define whitespace as any character less than or equal '\u0020'. TheisWhitespace()definition is slightly different and includes other Unicode space characters.Approach: The attached patch changes trim to use Character/isWhitespace. The isWhitespace version seems generally newer and more Unicode considerate so this was chosen over changing triml and trimr to match trim.
A few alternative implementations were considered with respect to longs, ints, etc. The patch opts to use the simplest possible code, eschewing any extreme performance measures. See the comments for more info if desired.
The patch also changes triml to only call .length on s once.
Patch: clj935-3.patch
Screened by: Stuart Sierra