weavejester / cljfmt

A tool for formatting Clojure code
Eclipse Public License 1.0
1.11k stars 120 forks source link

Refactor indents sorting to improve performance #239

Closed LuisThiamNye closed 3 years ago

LuisThiamNye commented 3 years ago

I've been profiling the code and found that the call to sort-by is very expensive and is called with a high frequency. I believe this call can safely be moved up the call hierarchy from custom-indent to indent so that it is only called once per call to indent.

Benchmark:

(require '[criterium.core :as crit])
(def sample (slurp "https://raw.githubusercontent.com/clojure/clojure/master/src/clj/clojure/core.clj"))
(crit/quick-bench 
 (cljfmt/reformat-string sample
                         {:indentation? true
                          :remove-consecutive-blank-lines? false
                          :remove-surrounding-whitespace? false
                          :insert-missing-whitespace? false
                          :remove-trailing-whitespace? false}))

Before:

Evaluation count : 6 in 6 samples of 1 calls.
             Execution time mean : 6.417959 sec
    Execution time std-deviation : 49.024102 ms
   Execution time lower quantile : 6.385542 sec ( 2.5%)
   Execution time upper quantile : 6.499747 sec (97.5%)
                   Overhead used : 7.166550 ns

Found 1 outliers in 6 samples (16.6667 %)
    low-severe   1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers

After:

Evaluation count : 6 in 6 samples of 1 calls.
             Execution time mean : 1.222197 sec
    Execution time std-deviation : 107.052812 ms
   Execution time lower quantile : 1.119392 sec ( 2.5%)
   Execution time upper quantile : 1.350514 sec (97.5%)
                   Overhead used : 7.166550 ns

Benchmark:

(crit/quick-bench
   (cljfmt/reformat-string sample
                           {:indentation? true
                            :remove-consecutive-blank-lines? true
                            :remove-surrounding-whitespace? true
                            :insert-missing-whitespace? true
                            :remove-trailing-whitespace? true}))

Before:

Evaluation count : 6 in 6 samples of 1 calls.
             Execution time mean : 6.803126 sec
    Execution time std-deviation : 110.233777 ms
   Execution time lower quantile : 6.734209 sec ( 2.5%)
   Execution time upper quantile : 6.983225 sec (97.5%)
                   Overhead used : 7.166550 ns

Found 1 outliers in 6 samples (16.6667 %)
    low-severe   1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers

After:

Evaluation count : 6 in 6 samples of 1 calls.
             Execution time mean : 1.536204 sec
    Execution time std-deviation : 145.444873 ms
   Execution time lower quantile : 1.398777 sec ( 2.5%)
   Execution time upper quantile : 1.692483 sec (97.5%)
                   Overhead used : 7.166550 ns
ericdallo commented 3 years ago

That looks very promising, that would help a lot clojure-lsp!

weavejester commented 3 years ago

Thanks for the patch. This looks fine to merge.

ericdallo commented 3 years ago

@weavejester could we release a patch version for this change?