Clojure JIRA

<< Back to previous view

[CLJ-1084] (object-array [1]) is ~3x slower than (object-array (rseq [1])) Created: 09/Oct/12 Updated: 20/Oct/12 Resolved: 20/Oct/12
Status:	Closed
Project:	Clojure
Component/s:	None
Affects Version/s:	Release 1.4
Fix Version/s:	Release 1.5

Type:

Defect

Priority:

Minor

Reporter:

Paul Stadig

Assignee:

Unassigned

Resolution:

Completed

Votes:

Labels:

patch,

Attachments:

0001-Make-PersistentVector-ChunkedSeq-implement-Counted.patch

CLJ-1084-tests.patch

Patch:

Code and Test

Approval:

Description

{{user=> (time (dotimes [_ 10000000] (object-array [1])))
"Elapsed time: 1178.116257 msecs"
nil
user=> (time (dotimes [_ 10000000] (object-array (rseq [1]))))
"Elapsed time: 422.42248 msecs"
nil}}

This appears to be because PersistentVector$ChunkedSeq does not implement Counted, so RT.count is iterating the ChunkedSeq to get its count.

Comments

Comment by Paul Stadig [ 09/Oct/12 2:11 PM ]

I don't believe this is Major priority, but I cannot edit the ticket after having created it.

Comment by Tassilo Horn [ 11/Oct/12 10:17 AM ]

This patch makes PersistentVector$ChunkedSeq implement Counted.

Performance before:

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.862259 msecs"
;"Elapsed time: 7228.72486 msecs"

Performance after (with the patch):

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.967301 msecs"
;"Elapsed time: 0.99391 msecs"

Also with Paul's test case.

Before:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 1668.346997 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 662.820591 msecs"

After:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 757.084577 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 680.602921 msecs"

Comment by Stuart Halloway [ 19/Oct/12 1:46 PM ]

Two patches to be applied together, the 10/19 patch adds tests and updates to latest test.generative.

[CLJ-1020] clojure.inspector/inspect-table gives up when first element of coll is nil Created: 02/Jul/12 Updated: 13/Sep/12
Status:	Open
Project:	Clojure
Component/s:	None
Affects Version/s:	Release 1.4
Fix Version/s:	None

Type:

Enhancement

Priority:

Trivial

Reporter:

Dimitrios Piliouras

Assignee:

Unassigned

Resolution:

Unresolved

Votes:

Labels:

patch,

Environment:

Ubuntu 12.04, Java 7, Clojure 1.4

Attachments:

clj-1020-inspect-table-skip-nil-rows-patch1.txt

inspector.patch

Patch:

Code

Description

clojure.inspector/inspect-table gives up when first element of coll is nil. The patch provided is rather trivial...instead of blindly choosing the first element (which might be nil), it would be more convenient to choose the first element that is NOT nil and use its keys for columns...a similar issue exists with clojure.pprint/print-table where the keys of the first element are used (if not provided explicitly). The same is not true for 'inspect-table' though. As a result, one cannot 'inspect' a collection of maps where the first element is nil. My (trivial) patch looks for the first element which is NOT nil and uses its keys instead. Maps have to have the same length anyway so no problems there...

Comments

Comment by Andy Fingerhut [ 12/Jul/12 1:01 PM ]

clj-1020-inspect-table-skip-nil-rows-patch1.txt of July 12, 2012 is identical to inspector.patch of July 2, 2012, except it is in the desired git format. Proper attribution is given to author Dimitrios Piliouras in the patch.

[CLJ-991] partition-by reducer Created: 10/May/12 Updated: 04/Mar/13
Status:	Open
Project:	Clojure
Component/s:	None
Affects Version/s:	None
Fix Version/s:	None

Type:

Enhancement

Priority:

Minor

Reporter:

Kevin Downey

Assignee:

Unassigned

Resolution:

Unresolved

Votes:

Labels:

enhancement, patch, patch,

Attachments:

reducer-partition-by2.diff

reducer-partition-by3.diff

reducer-partition-by4.diff

reducer-partition-by.diff

Patch:

Code and Test

Comments

Comment by Rich Hickey [ 14/Aug/12 1:52 PM ]

I'd like to see something much faster than this.

Comment by Kevin Downey [ 15/Aug/12 1:58 AM ]

For reference here is a benchmark of a non-reducers (seq based) process that uses partition-by

user=> (def x (vec (range 1e6)))
#'user/x
user=> (bench (reduce + (map count (partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 60
             Execution time mean : 1.072157 sec  95.0% CI: (1.070606 sec, 1.073381 sec)
    Execution time std-deviation : 165.818282 ms  95.0% CI: (163.873585 ms, 168.271261 ms)
         Execution time lower ci : 972.562000 ms  95.0% CI: (972.562000 ms, 973.301850 ms)
         Execution time upper ci : 1.419148 sec  95.0% CI: (1.419148 sec, 1.419148 sec)

Found 7 outliers in 60 samples (11.6667 %)
	low-severe	 2 (3.3333 %)
	low-mild	 5 (8.3333 %)
 Variance from outliers : 85.8489 % Variance is severely inflated by outliers
nil
user=>

Same again using r/partition-by from reducer-partition-by.diff

user=> (bench (r/reduce + (r/map count (r/partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 60
             Execution time mean : 1.418350 sec  95.0% CI: (1.417738 sec, 1.418948 sec)
    Execution time std-deviation : 66.736477 ms  95.0% CI: (66.186568 ms, 67.610777 ms)
         Execution time lower ci : 1.370419 sec  95.0% CI: (1.370419 sec, 1.370419 sec)
         Execution time upper ci : 1.544151 sec  95.0% CI: (1.544151 sec, 1.544156 sec)

Found 10 outliers in 60 samples (16.6667 %)
	low-severe	 2 (3.3333 %)
	low-mild	 8 (13.3333 %)
 Variance from outliers : 33.5591 % Variance is moderately inflated by outliers
nil
user=>

(using https://github.com/hugoduncan/criterium for benchmarking)

Comment by Kevin Downey [ 15/Aug/12 2:17 AM ]

same again for r/partition-by from reducers-partition-by2.diff

user=> (bench (r/reduce + (r/map count (r/partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 180
             Execution time mean : 307.596806 ms  95.0% CI: (307.271339 ms, 307.961550 ms)
    Execution time std-deviation : 34.060809 ms  95.0% CI: (33.613169 ms, 34.416837 ms)
         Execution time lower ci : 285.339333 ms  95.0% CI: (285.339333 ms, 285.339333 ms)
         Execution time upper ci : 385.087950 ms  95.0% CI: (385.087950 ms, 385.087950 ms)

Found 10 outliers in 60 samples (16.6667 %)
	low-severe	 4 (6.6667 %)
	low-mild	 6 (10.0000 %)
 Variance from outliers : 73.8053 % Variance is severely inflated by outliers
nil
user=>

same again driven using r/fold (for grins) instead of r/reduce

user=> (bench (r/fold + (r/map count (r/partition-by #(or (zero? (mod % 3)) (zero? (mod % 5))) x))))
Evaluation count             : 360
             Execution time mean : 215.214486 ms  95.0% CI: (214.915417 ms, 215.664236 ms)
    Execution time std-deviation : 36.588464 ms  95.0% CI: (36.305548 ms, 36.847846 ms)
         Execution time lower ci : 185.575000 ms  95.0% CI: (185.575000 ms, 185.575000 ms)
         Execution time upper ci : 287.605175 ms  95.0% CI: (286.547833 ms, 287.605175 ms)

Found 6 outliers in 60 samples (10.0000 %)
	low-severe	 3 (5.0000 %)
	low-mild	 3 (5.0000 %)
 Variance from outliers : 87.6303 % Variance is severely inflated by outliers
nil
user=>

reducers-partition-by2.diff is faster, but I am not wild about introducing a type and a protocol. also not sure about the CollFold impl.

Comment by Rich Hickey [ 15/Aug/12 6:58 AM ]

Let's leave fold out for this first patch, please.

Comment by Kevin Downey [ 15/Aug/12 2:34 PM ]

reducer-partition-by3.diff is a cleaned up version of reducer-partition-by2.diff without fold

Comment by Devin Walters [ 20/Oct/12 7:07 PM ]

Per Andy Fingerhut's email reducer-partition-by3.diff was failing to apply. This patch should apply cleanly to current master.

Comment by Andy Fingerhut [ 01/Nov/12 6:59 PM ]

Presumptuously changing Approval from Incomplete to None, since the reason for its being marked Incomplete seems to have been addressed with the latest patch.

Comment by Kevin Downey [ 04/Mar/13 2:49 PM ]

should this be assigned to someone?

[CLJ-963] Support pretty printing namespace declarations under code-dispatch Created: 29/Mar/12 Updated: 01/Sep/12 Resolved: 01/Sep/12
Status:	Closed
Project:	Clojure
Component/s:	None
Affects Version/s:	Release 1.2, Release 1.3
Fix Version/s:	Release 1.5

Type:

Defect

Priority:

Minor

Reporter:

Tom Faulhaber

Assignee:

Stuart Sierra

Resolution:

Completed

Votes:

Labels:

patch,

Environment:

all

Attachments:

pprint-ns-patch.diff

Patch:

Code and Test

Approval:

Waiting On:

Rich Hickey

Description

Currently when you print code with an (ns ) declaration in it, the ns declaration comes out kind of messy. This patch makes that beautiful.

Comments

Comment by Stuart Sierra [ 24/Aug/12 8:30 AM ]

Screened.

It's not perfect, but an improvement.

Before the patch:

user=> (with-pprint-dispatch code-dispatch (pprint '(ns com.some-example.my-app (:require clojure.string [clojure.set :as set] [clojure.java.io :refer (file reader)]))))
(ns
  com.some-example.my-app
  (:require
    clojure.string
    [clojure.set :as set]
    [clojure.java.io :refer (file reader)]))
nil

After the patch:

user=> (with-pprint-dispatch code-dispatch (pprint '(ns com.some-example.my-app (:require clojure.string [clojure.set :as set] [clojure.java.io :refer (file reader)]))))
(ns com.some-example.my-app
  (:require clojure.string [clojure.set :as set]
            [clojure.java.io :refer (file reader)]))
nil

[CLJ-953] drop-while doc string wrong, nil instead of logical false Created: 15/Mar/12 Updated: 30/Mar/12 Resolved: 30/Mar/12
Status:	Closed
Project:	Clojure
Component/s:	None
Affects Version/s:	Release 1.3
Fix Version/s:	Release 1.4

Type:

Enhancement

Priority:

Trivial

Reporter:

Eric Schoonover

Assignee:

Alan Dipert

Resolution:

Completed

Votes:

Labels:

documentation, patch,

Attachments:

take-while_doc_string_CLJ-953.diff

Patch:

Code

Approval:

Description

"Returns a lazy sequence of the items in coll starting from the first
-  item for which (pred item) returns nil."
+  item for which (pred item) returns logical false."

[CLJ-886] java.io/do-copy can garble multibyte characters Created: 28/Nov/11 Updated: 23/Mar/12 Resolved: 23/Mar/12
Status:	Closed
Project:	Clojure
Component/s:	None
Affects Version/s:	Release 1.3
Fix Version/s:	Release 1.4

Type:

Defect

Priority:

Major

Reporter:

Jeff Palmucci

Assignee:

Unassigned

Resolution:

Completed

Votes:

Labels:

patch,

Environment:

all

Attachments:

clj-886.diff

CLJ-886-fix2.patch

Patch:

Code and Test

Approval:

Waiting On:

Rich Hickey

Description

See comments in fix:

(defmethod do-copy [InputStream Writer] [#^InputStream input #^Writer output opts]
;; WRONG! if the buffer boundry falls in the middle of a multibyte character, we will get garbled results.
#_
(let [#^"[B" buffer (make-array Byte/TYPE (buffer-size opts))]
(loop []
(let [size (.read input buffer)]
(when (pos? size)
(let [chars (.toCharArray (String. buffer 0 size (encoding opts)))]
(do (.write output chars)
(recur)))))))
;; here we decode the characters before stuffing them into the buffer
(let [#^"[C" buffer (make-array Character/TYPE (buffer-size opts))
in (InputStreamReader. input (encoding opts))]
(loop []
(let [size (.read in buffer 0 (alength buffer))]
(if (pos? size)
(do (.write output buffer 0 size)
(recur)))))))

Comments

Comment by Jeff Palmucci [ 05/Dec/11 11:23 AM ]

Make patch file per http://clojure.org/patches. Also sent in my CA.

Comment by Jeff Palmucci [ 19/Dec/11 8:53 AM ]

Any reason why this hasn't been applied yet? It's a pretty serious error. Just wondering if I've submitted anything incorrectly. Thanks.

Comment by Andy Fingerhut [ 09/Feb/12 8:13 PM ]

~~CLJ-886~~-fix2.patch includes Jeff Palmucci's fix, plus another one found while enhancing clojure.java.io's unit tests to fail with the original code. Tested on Mac OS X 10.6.8 with java 1.6.0_29 and Ubuntu Linux 11.04 with JVM 1.7.0_02.

Comment by Jeff Palmucci [ 13/Feb/12 7:29 AM ]

Andy's fix is good. Please use the fix2 patch instead of the original.

Comment by Stuart Sierra [ 17/Feb/12 2:55 PM ]

Screened. Patch applies successfully and the tests seem to be complete.

[CLJ-879] Allow :require to support a :refer clause Created: 17/Nov/11 Updated: 17/Feb/12 Resolved: 17/Feb/12
Status:	Closed
Project:	Clojure
Component/s:	None
Affects Version/s:	None
Fix Version/s:	None

Type:

Enhancement

Priority:

Minor

Reporter:

Phil Hagelberg

Assignee:

Phil Hagelberg

Resolution:

Completed

Votes:

Labels:

patch,, test

Attachments:

0001-Allow-require-to-take-a-refer-option.patch

Patch:

Code and Test

Approval:

Description

There's been discussion previously about the complexity of the ns
macro. In particular the fact that :use causes all vars to be referred
by default is widely seen as unfortunate, and the distinction between
use and require is a source of conceptual overhead for people new to
the language.

We can't change the fact that :use refers everything by default
without breaking lots of existing code. But it would be possible to
enhance :require to support referring specified vars, leaving us free
to deprecate or otherwise discourage the use of :use.

Clojure-dev thread discussing this: http://groups.google.com/group/clojure-dev/browse_thread/thread/91b708ddb909affd

Comments

Comment by Phil Hagelberg [ 25/Nov/11 2:52 PM ]

Patch containing implementation, test, and documentation.

Comment by Phil Hagelberg [ 06/Jan/12 2:21 PM ]

Any chance we could get some discussion going on this?

Comment by Devin Torres [ 14/Jan/12 1:12 AM ]

I'd love to see this discussed.

Comment by Phil Hagelberg [ 27/Jan/12 4:42 PM ]

So... how about it? Thoughts?

Comment by Kevin Downey [ 17/Feb/12 11:30 AM ]

patch still applies cleanly to master, tests pass

there is a warning when you run the tests though:

[java] Testing clojure.test-clojure.keywords
[java]
[java] Testing clojure.test-clojure.load
[java] WARNING: with-bindings already refers to: #'clojure.core/with-bindings in namespace: clojure.test-clojure.require-scratch, being replaced by: #'clojure.main/with-bindings
[java]
[java] Testing clojure.test-clojure.logic
[java]
[java] Testing clojure.test-clojure.macros

Comment by Stuart Sierra [ 17/Feb/12 1:36 PM ]

Vetted. Patch is good, although it needs docstring updates in 'require'

Comment by Stuart Sierra [ 17/Feb/12 1:55 PM ]

Not Vetted. I can't remember what the names mean. Ready for Rich. That's "Test" right?

Comment by Stuart Sierra [ 17/Feb/12 1:59 PM ]

My mistake. The docstring is included in 'require', but not in 'refer', which also changed. But the public API of 'refer' did not change. So this patch is good.

[CLJ-1084] (object-array [1]) is ~3x slower than (object-array (rseq [1])) Created: 09/Oct/12 Updated: 20/Oct/12 Resolved: 20/Oct/12

[CLJ-1020] clojure.inspector/inspect-table gives up when first element of coll is nil Created: 02/Jul/12 Updated: 13/Sep/12

[CLJ-991] partition-by reducer Created: 10/May/12 Updated: 04/Mar/13

[CLJ-963] Support pretty printing namespace declarations under code-dispatch Created: 29/Mar/12 Updated: 01/Sep/12 Resolved: 01/Sep/12

[CLJ-953] drop-while doc string wrong, nil instead of logical false Created: 15/Mar/12 Updated: 30/Mar/12 Resolved: 30/Mar/12

[CLJ-886] java.io/do-copy can garble multibyte characters Created: 28/Nov/11 Updated: 23/Mar/12 Resolved: 23/Mar/12

[CLJ-879] Allow :require to support a :refer clause Created: 17/Nov/11 Updated: 17/Feb/12 Resolved: 17/Feb/12