gfredericks / seventy-one

71 in Clojure
Eclipse Public License 1.0
39 stars 9 forks source link

Add ^:const metadata for efficiency #2

Closed aengelberg closed 4 years ago

aengelberg commented 9 years ago

For a speed comparison I tried this in the REPL:

(def ^:const seventy-one
  "The number seventy-one, i.e. 71."
  71)

(def seventy-one-not-const 71)
com.gfredericks.seventy-one> (time (dotimes [i 1e8] (* i seventy-one-not-const)))
"Elapsed time: 1823.769064 msecs"
nil
com.gfredericks.seventy-one> (time (dotimes [i 1e8] (* i seventy-one)))
"Elapsed time: 190.361232 msecs"
nil

You might want to try benchmarking with criterium to be absolute sure of the speed difference, but it's pretty obvious this is an essential performance boost for those looking to use this in production.

suzaku commented 9 years ago

:+1:

gfredericks commented 9 years ago

The tradeoff here is that we lose dynamic-runtime features, in particular we can't mock the behavior of seventy-one in tests using with-redefs, which I expect is pretty useful.

I'll have to give this one some thought.

danielcompton commented 9 years ago

Can you make sure these are laptop benchmarks?

tcrayford commented 9 years ago

Did you run those benchmarks with lein's default jvm optimizations? Lein turns off most of the standard jvm optimizations, which can severely inhibit inlining through vars. See https://github.com/dakrone/cheshire/pull/75 for another example.

gfredericks commented 9 years ago

What would folks think about having a separate var for this so people could opt-in to the restricted functionality if they wanted to? Maybe call it fast-seventy-one?

dcmoore commented 9 years ago

@gfredericks, I would love to see that feature.

marcomorain commented 5 years ago

In Spec-ulation, Rich suggests that adding a number to the end of names is a neat way to version things. How about the new var is named this this way?

seventy-one-two?

gfredericks commented 5 years ago

Or just seventy-one-const

aengelberg commented 4 years ago

Closing this since we don't seem to be approaching a resolution on making these changes ready to merge, but feel free to re-open if someone else wants to take it over the line.