Performance improvments on beginning and end.

[niclasnilsson]

Background

We've been using tick for a very interval intensive application for some years now and it's been working well. Due to some API changes, we haven't upgraded tick since 0.4.32, but now did some refactorings and upgraded to 0.7.2. However, our acceptance tests started to run extremely slow, to a point that we would not be able to use a new tick in production, which would be a pity.

So I started to dig in, and found that the bottleneck was the new implementation of beginning and end that checks if v satisfies p/TimeSpan before calling.

(defn beginning "the beginning of the range of ITimeSpan v or v" [v] (if (satisfies? p/ITimeSpan v) (p/beginning v) v))

I changed this to this:

(defn beginning "the beginning of the range of ITimeSpan v or v" [v]
  (try (p/beginning v) (catch #?(:clj Exception :cljs js/Error) _e v)))

Large test benchmarks

With this change, performance went back to almost what it was before. Here are some numbers:

;; With tick 0.4.32
;;
;; (time (clojure.test/run-tests))
;;
;; Testing <application-namespace>.acceptance-test
;;
;; Ran 26 tests containing 26 assertions.
;; 0 failures, 0 errors.
;; "Elapsed time: 16185.514044 msecs"
;;
;; result:
;;
;; {:error 0 :fail 0 :pass 26 :test 26 :type :summary}

;; ----------------------------------------------------------

;; With original tick 0.7.2
;;
;; (time (clojure.test/run-tests))
;;
;; Testing <application-namespace>.acceptance-test
;;
;; Ran 26 tests containing 26 assertions.
;; 0 failures, 0 errors.
;; "Elapsed time: 244285.029638 msecs"
;;
;; result:
;;
;; {:error 0 :fail 0 :pass 26 :test 26 :type :summary}

;; ----------------------------------------------------------

;; With patched tick 0.7.2
;;
;; (time (clojure.test/run-tests))
;;
;; Testing <application-namespace>.acceptance-test
;;
;; Ran 26 tests containing 26 assertions.
;; 0 failures, 0 errors.
;; "Elapsed time: 16736.042017 msecs"
;;
;; result:
;;
;; {:error 0 :fail 0 :pass 26 :test 26 :type :summary}

Microbenchmarks

Here are the microbenchmarks for the different versions:

;;
;; With tick 0.4.32
;;

(use 'criterium.core)
(require '[tick.alpha.api :as t])
(def t1 (t/new-interval (t/date "2021-02-01") (t/date "2021-02-05")))

(with-progress-reporting (bench (t/beginning t1) :verbose))

;;
;; Evaluation count : 1146487740 in 60 samples of 19108129 calls.
;; Execution time sample mean : 45.372086 ns
;; Execution time mean : 45.373254 ns
;; Execution time sample std-deviation : 0.220612 ns
;; Execution time std-deviation : 0.234171 ns
;; Execution time lower quantile : 45.063637 ns ( 2.5%)
;; Execution time upper quantile : 45.853972 ns (97.5%)
;; Overhead used : 7.744911 ns
;;
;; Found 3 outliers in 60 samples (5.0000 %)
;; low-severe 1 (1.6667 %)
;; low-mild 2 (3.3333 %)
;; Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
;;

;;
;; With tick 0.7.2 (500x slower)
;;

(use 'criterium.core)
(require '[tick.core :as t])
(require '[tick.alpha.interval :as ti])

(def t1 (ti/new-interval (t/date "2021-02-01") (t/date "2021-02-05")))

(with-progress-reporting (bench (t/beginning t1) :verbose))

;;
;; Evaluation count : 2620980 in 60 samples of 43683 calls.
;; Execution time sample mean : 22.829573 µs
;; Execution time mean : 22.836290 µs
;; Execution time sample std-deviation : 395.071874 ns
;; Execution time std-deviation : 395.633990 ns
;; Execution time lower quantile : 22.682359 µs ( 2.5%)
;; Execution time upper quantile : 22.996234 µs (97.5%)
;; Overhead used : 7.746340 ns
;;
;; Found 4 outliers in 60 samples (6.6667 %)
;; low-severe 2 (3.3333 %)
;; low-mild 2 (3.3333 %)
;; Variance from outliers : 6.2811 % Variance is slightly inflated by outliers
;;

;;
;; With patched tick 0.7.2 (similar to 0.4.32)
;;

(use 'criterium.core)
(require '[tick.core :as t])
(require '[tick.alpha.interval :as ti])

(def t1 (ti/new-interval (t/date "2021-02-01") (t/date "2021-02-05")))

(with-progress-reporting (bench (t/beginning t1) :verbose))

;;
;; Evaluation count : 1153781880 in 60 samples of 19229698 calls.
;; Execution time sample mean : 45.005852 ns
;; Execution time mean : 45.019090 ns
;; Execution time sample std-deviation : 0.943030 ns
;; Execution time std-deviation : 0.948350 ns
;; Execution time lower quantile : 44.036791 ns ( 2.5%)
;; Execution time upper quantile : 46.372050 ns (97.5%)
;; Overhead used : 7.749936 ns

The API for beginning/end in general

On a higher level, I don't see the reason for having beginning and end returning v if v doesn't match the protocol. If I would call (t/beginning 0.5) with 0.4.32, it would throw an exception (which feels very reasonable), but in 0.7.2 it would instead return 0.5.

It's an obvious programming error, and it would make more sense to me to get an exception if it's not an ITimeSpan. I don't fully see the use case for silently returning what was sent in when it's not an ITimeSpan?

In the spirit of this, I did try to change beginning/end to just call p/beginning and p/end and not returning v if it fails/won't work, but it broke quite a lot of tests, so the try/catch is at least compatible with 0.7.2 behaviour, but I do feel that an exception for a non ITimeSpan argument would be more appropriate.

; with 0.4.32
(t/beginning 0.5)

Execution error (IllegalArgumentException) at tick.protocols/eval20902$fn$G (protocols.cljc:63).
No implementation of method: :time of protocol: #'tick.protocols/IExtraction found for class: java.lang.Double

; with 0.7.2
(t/beginning 0.5)

result: 0.5

[henryw374]

Hi. Thanks for looking into this.

re On a higher level, I don't see the reason for having beginning and end returning v if v doesn't match the protocol

the reason is that until now ITimespan has been extended to things like Instant, where beginning and end just returned the interval. to me, that doesnt really make intuitive sense - or at least I can imagine someone has a use case where the end is the next nano on the timeline. so... in the interests of backward compatibility I changed how beginning/end work but stopped extending ITimespan to those, so anyone with that use case can extend it how they like.

I mean... I could extend ITimespan to the stuff it was originally extended to, and ppl could override as required. but... what you've done works. I might just add a comment in the code as to why it is like that.

[henryw374]

having second thoughts now. maybe I'll just extend ITimespan as it was before - ie keep backward compatibility even if it doesnt really make sense... and then can remove any check from those fns.

sound ok?

[niclasnilsson]

If you extend ITimeSpan to work for instants (if that's needed), then it will at least not work for doubles, strings and other unrelated things, so I suppose that's better? (If I understood you correctly?)

[henryw374]

yes that's right.

the other alternative is to use your impl and in the catch, check if Temporal or TemporalAmount ... and re-throw if not either. but even that feels a bit too weird

[niclasnilsson]

Ok, then I vote for extend ITimeSpan for the things needed (even if instants is wierd) and leave the rest with the natural exception .

[henryw374]

ok I have released 0.7.3 where beginning+end just call through to the protocol. hope that puts your performance back to how it was