Clojure

slurp should not read one character at a time

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: Backlog
  • Fix Version/s: Release 1.4
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Incomplete

Description

A discussion of slurp performance came up on the mailing list. The attached patch makes slurp use a 4kb buffer size instead of reading one character at a time, resulting in a significant performance improvement.

Activity

Hide
Assembla Importer added a comment -

stu said: Updating tickets (#389, #371)

Show
Assembla Importer added a comment - stu said: Updating tickets (#389, #371)
Hide
Assembla Importer added a comment -

aaron said: Patch does not apply. It is not in the proper format. Please take a look at http://clojure.org/patches and resubmit. I will continue to review the code itself against the current master.

Show
Assembla Importer added a comment - aaron said: Patch does not apply. It is not in the proper format. Please take a look at http://clojure.org/patches and resubmit. I will continue to review the code itself against the current master.
Hide
Aaron Bedra added a comment -

Contacted original poster about this to inform him of the move to JIRA and ensure interest in this patch

Show
Aaron Bedra added a comment - Contacted original poster about this to inform him of the move to JIRA and ensure interest in this patch
Hide
Aaron Bedra added a comment -

The patch has been reformatted and submitted.

Show
Aaron Bedra added a comment - The patch has been reformatted and submitted.
Aaron Bedra made changes -
Field Original Value New Value
Reporter Aaron Bedra [ aaron ]
Priority Blocker [ 1 ]
Patch Code
Aaron Bedra made changes -
Attachment 0389_buffered_slurp.diff [ 10018 ]
Hide
Jürgen Hötzel added a comment -

Maybe duplicate: CLJ-668

Show
Jürgen Hötzel added a comment - Maybe duplicate: CLJ-668
Hide
Aaron Bedra added a comment - - edited

CLJ-668 is a duplicate of this ticket. I have tested your patch and would like to consider it as a resolution to this ticket along with closing CLJ-668 as a duplicate. Can you please submit a patch to this ticket instead of a link to github? I am going to test all of the patches presented on OSX, Linux, EC2, and Windows.

Show
Aaron Bedra added a comment - - edited CLJ-668 is a duplicate of this ticket. I have tested your patch and would like to consider it as a resolution to this ticket along with closing CLJ-668 as a duplicate. Can you please submit a patch to this ticket instead of a link to github? I am going to test all of the patches presented on OSX, Linux, EC2, and Windows.
Hide
Jürgen Hötzel added a comment - - edited

Seems like the changes are already applied by your previous commit:

https://github.com/clojure/clojure/commit/cbd789d1a5b472d92b91f2fe0e273f48c2583483

Just a leftover StringBuilder Object (patch attached)

Show
Jürgen Hötzel added a comment - - edited Seems like the changes are already applied by your previous commit: https://github.com/clojure/clojure/commit/cbd789d1a5b472d92b91f2fe0e273f48c2583483 Just a leftover StringBuilder Object (patch attached)
Jürgen Hötzel made changes -
Hide
Aaron Bedra added a comment -

Yes, that was a mistake. It should not have gone in under #441. I was doing some testing and somehow I didn't properly revert what I was working on. Please consider that a mistake for now and include a patch here so you can get credit for your work.

Show
Aaron Bedra added a comment - Yes, that was a mistake. It should not have gone in under #441. I was doing some testing and somehow I didn't properly revert what I was working on. Please consider that a mistake for now and include a patch here so you can get credit for your work.
Hide
Jürgen Hötzel added a comment -

Thanks. Patch enclosed.

Show
Jürgen Hötzel added a comment - Thanks. Patch enclosed.
Jürgen Hötzel made changes -
Hide
Aaron Bedra added a comment -

Thanks. I am going to be testing the various ideas on multiple platforms to see which (if any) is the best fit.

Show
Aaron Bedra added a comment - Thanks. I am going to be testing the various ideas on multiple platforms to see which (if any) is the best fit.
Aaron Bedra made changes -
Approval Test Vetted
Aaron Bedra made changes -
Fix Version/s Approved Backlog [ 10034 ]
Fix Version/s Release.Next [ 10038 ]
Affects Version/s Approved Backlog [ 10034 ]
Priority Blocker [ 1 ] Minor [ 4 ]
Rich Hickey made changes -
Fix Version/s Release 1.4 [ 10040 ]
Fix Version/s Approved Backlog [ 10034 ]
Hide
Aaron Bedra added a comment - - edited

This patch carries a performance hit. For example:

Clojure 1.4.0

File Size best timed 10x slurp
7.3 MB 2960.363 msecs
260 KB 105.255 msecs

With Patch

File Size best timed 10x slurp
7.3 MB 3700.406 msecs
260 KB 129.311 msecs

This was tested with

(time (dotimes [_ 10] (slurp "file")))

with each execution of this form done 20 times.

Show
Aaron Bedra added a comment - - edited This patch carries a performance hit. For example: Clojure 1.4.0
File Size best timed 10x slurp
7.3 MB 2960.363 msecs
260 KB 105.255 msecs
With Patch
File Size best timed 10x slurp
7.3 MB 3700.406 msecs
260 KB 129.311 msecs
This was tested with (time (dotimes [_ 10] (slurp "file"))) with each execution of this form done 20 times.
Aaron Bedra made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Aaron Bedra made changes -
Status In Progress [ 3 ] Open [ 1 ]
Aaron Bedra made changes -
Resolution Declined [ 2 ]
Status Open [ 1 ] Resolved [ 5 ]
Stuart Halloway made changes -
Status Resolved [ 5 ] Closed [ 6 ]
Alex Miller made changes -
Affects Version/s Approved Backlog [ 10034 ]
Affects Version/s Backlog [ 10035 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: