Clojure

Remove reflection in pprint and cl-format to improve performance

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.8
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Ok

Description

There are many occurrences of reflection in the pprint implementation.

By eliminating all of them, I ran one benchmark of pprint'ing a Clojure map that resulted in a 300 Kbyte output. After eliminating reflection, the elapsed time to pprint was reduced by 18% (about 14.0 sec down to about 11.5 sec) on a recent model MacBook Pro.

Patch: clj-1259-2.patch
Screened by: Alex Miller

  1. clj-1259-1.txt
    09/Sep/13 11:36 PM
    11 kB
    Andy Fingerhut
  2. clj-1259-2.patch
    09/Oct/15 8:59 AM
    20 kB
    Alex Miller

Activity

Hide
Andy Fingerhut added a comment -

Michael, I have attached a minimal patch to CLJ-1827 for the one reflection warning still in pretty_writer.clj.

I did not save a copy of your patch before Alex removed it from this ticket, so cannot create a patch similar to yours for CLJ-1827. If you attach your patch as is to CLJ-1827, I could look into updating it for latest master.

Show
Andy Fingerhut added a comment - Michael, I have attached a minimal patch to CLJ-1827 for the one reflection warning still in pretty_writer.clj. I did not save a copy of your patch before Alex removed it from this ticket, so cannot create a patch similar to yours for CLJ-1827. If you attach your patch as is to CLJ-1827, I could look into updating it for latest master.
Hide
Alex Miller added a comment -

I created CLJ-1827 to track this - feel free to drop a patch there.

Show
Alex Miller added a comment - I created CLJ-1827 to track this - feel free to drop a patch there.
Hide
Andy Fingerhut added a comment -

Michael, would you be interested in creating the ticket and creating the patch for it? If not, I can get to it in a week or two.

Show
Andy Fingerhut added a comment - Michael, would you be interested in creating the ticket and creating the patch for it? If not, I can get to it in a week or two.
Hide
Alex Miller added a comment -

Please create a new ticket for the new warning...

Show
Alex Miller added a comment - Please create a new ticket for the new warning...
Hide
Andy Fingerhut added a comment -

Michael, looks like that reflection warning is in code introduced after I wrote my patch. Now that the v2 patch has been committed, since it includes setting warn-on-reflection to true for the pretty_writer.clj source file, there is now that one reflection warning printed during builds of the latest master. Eliminating that warning might be enough motivation for the Clojure core team to get a fix for it into a later 1.8.0-beta, but of course that is their decision.

Show
Andy Fingerhut added a comment - Michael, looks like that reflection warning is in code introduced after I wrote my patch. Now that the v2 patch has been committed, since it includes setting warn-on-reflection to true for the pretty_writer.clj source file, there is now that one reflection warning printed during builds of the latest master. Eliminating that warning might be enough motivation for the Clojure core team to get a fix for it into a later 1.8.0-beta, but of course that is their decision.
Hide
Alex Miller added a comment -

Michael, the -2 patch is the one that has been screened and ok'ed by Rich - any other change will need to go into a new ticket. I've removed the -v3 patch to reduce confusion when Stu goes to apply.

Show
Alex Miller added a comment - Michael, the -2 patch is the one that has been screened and ok'ed by Rich - any other change will need to go into a new ticket. I've removed the -v3 patch to reduce confusion when Stu goes to apply.
Hide
Michael Blume added a comment -

Added v3 patch which fixes one more bit of reflection around line 419, and moves a few type hints to the return types of helpers so they don't have to be duplicated. Kept expanded context, attribution unchanged.

Show
Michael Blume added a comment - Added v3 patch which fixes one more bit of reflection around line 419, and moves a few type hints to the return types of helpers so they don't have to be duplicated. Kept expanded context, attribution unchanged.
Hide
Alex Miller added a comment -

Added -2 patch that changes commit message and adds diff context, no semantic changes, attribution retained.

Show
Alex Miller added a comment - Added -2 patch that changes commit message and adds diff context, no semantic changes, attribution retained.
Hide
Andy Fingerhut added a comment -

Patch clj-1259-1.txt eliminates all occurrences of reflection in pprint, and all files loaded from pprint.clj. It also sets warn-on-reflection to true for those files, in hopes of making it more obvious if a new use of reflection is added there.

Show
Andy Fingerhut added a comment - Patch clj-1259-1.txt eliminates all occurrences of reflection in pprint, and all files loaded from pprint.clj. It also sets warn-on-reflection to true for those files, in hopes of making it more obvious if a new use of reflection is added there.

People

Vote (1)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: