Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

loop should retain primitive int or float without widening

Description

Problem

A loop var that starts as a primitive int is unnecessarily widened to a long.

Context

In the following example:

(defn incrementer [n] (let [n (int n)] (loop [i (int 0)] (if (< i n) (recur (unchecked-inc-int i)) i))))

the loop-local starts as an int when just a local but is widened to a long in the recur. It should be possible to retain the primitive int (or alternately float) type on the recur, rather than widening to long (or double).

The compiler code that is promoting the int seems to be:
https://github.com/clojure/clojure/blob/a19c36927598677c32099dabd0fdb9d3097df259/src/jvm/clojure/lang/Compiler.java#L6377-L6380

Proposed: remove useless widening on loop bindings

Patch: clj-1905-2.patch

Screened by: Alex Miller. My main open question here is: do we want to support primitive int/float loop vars?

Environment

Possibly older Clojure versions (but not verified).

Attachments

2
  • 12 Dec 2018, 06:46 PM
  • 02 Apr 2016, 03:55 PM

Activity

Show:

Alex Miller December 12, 2018 at 6:46 PM

-2 patch rebased to apply to master, no changes, retains attribution

Nicola Mometto April 2, 2016 at 3:55 PM

Attached a patch that removes the useless widenings done by the compiler on loop bindings, here's a benchmark demonstrating the speedup gained when areducing over int-arrays:

Before patch:

Clojure 1.8.0 user=> (use 'criterium.core) nil user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i))))) Evaluation count : 64260 in 60 samples of 1071 calls. Execution time mean : 954.009929 µs Execution time std-deviation : 20.292809 µs Execution time lower quantile : 926.331747 µs ( 2.5%) Execution time upper quantile : 1.009189 ms (97.5%) Overhead used : 1.840681 ns Found 4 outliers in 60 samples (6.6667 %) low-severe 4 (6.6667 %) Variance from outliers : 9.4244 % Variance is slightly inflated by outliers nil

After patch:

Clojure 1.9.0-master-SNAPSHOT user=> (use 'criterium.core) nil user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i))))) Evaluation count : 68640 in 60 samples of 1144 calls. Execution time mean : 870.462532 µs Execution time std-deviation : 13.100790 µs Execution time lower quantile : 852.357513 µs ( 2.5%) Execution time upper quantile : 896.531529 µs (97.5%) Overhead used : 1.844045 ns Found 1 outliers in 60 samples (1.6667 %) low-severe 1 (1.6667 %) Variance from outliers : 1.6389 % Variance is slightly inflated by outliers nil

Nicola Mometto March 30, 2016 at 2:51 PM

I edited the title as the bug is in `loop`, not `recur`

Alex Miller March 29, 2016 at 6:30 PM

I'll leave this one in here but I'm going to treat it as an enhancement to current behavior. I think there's a good chance that Rich will just say this is working as intended.

I don't think the example is a very good one though and would welcome a better example. The reservations regarding unchecked-inc-int do not seem correct or valid to me (as usage should be fine on longs and is not designed for perf reasons anyways). A good example would should usage of a Java api in a loop where int-indexing and int-math gives better performance.

Nicola Mometto March 29, 2016 at 6:19 PM

I don't think we need to worry about fn recur targets at all, given that the only possible primitive bindings there are either long or double, and int/floats would get widened anyway, but good point, the tests in a patch for this ticket need to be sure that case is indeed handled.

RE: floats – I recall people complaining about bad support for floats when using clojure for graphical processing.

Even if admittedly a weak argument, I'm always of the idea that we should strike to be as consistent as possible. I don't think anybody would expect let/loop locals to behave differently, or differences between primitive types (other than the documented limitation about long/double being the only working prim types for fn arguments/return vals)

Details

Assignee

Reporter

Labels

Approval

Screened

Patch

Code and Test

Priority

Affects versions

Created March 29, 2016 at 4:37 PM
Updated September 14, 2021 at 12:28 PM

Flag notifications