Skip to:

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

Reducer instances hold onto the head of seqs

Description

(This ticket started life as CLJ-1250, was committed in 1.8.0-alpha2, pulled out after alpha4, and this is the new version that fixes the logic about whether in a tail call as well as addresses direct linking added in 1.8.0-alpha3.)

Problem: Original example was with reducers holding onto the head of a lazy seq:

(time (reduce + 0 (map identity (range 1e8)))) ;; works (time (reduce + 0 (r/map identity (range 1e8)))) ;; oome from holding head of range

Trickier example from CLJ-1250 that doesn't clear `this` in nested loop:

(let [done (atom false) f (future-call (fn inner [] (while (not @done) (loop [found []] (println (conj found 1))))))] (doseq [elem [:a :b :c :done]] (println "queue write " elem)) (reset! done true) @f)

Problem: #'reducer closes over a collection in order to reify CollReduce, and the closed-over collection reference is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Approach: When invoking a method in a tail call, clear 'this' prior to invoking.

The criteria for when a tail call is a safe point to clear 'this':

1) Must be in return position
2) Not in a try block (might need 'this' during catch/finally)
3) Not direct linked

Return position (#1) isn't simply (context == C.RETURN) because loop bodies are always parsed in C.RETURN context

A new dynvar METHOD_RETURN_CONTEXT tracks whether an InvokeExpr in tail position can directly leave the body of the compiled java method. It is set to RT.T in the outermost parsing of a method body and invalidated (set to null) when a loop body is being parsed where the context for the loop expression is not RETURN parsed. Added clear in StaticInvokeExpr as that is now a thing with direct linking again.

Removes calls to emitClearLocals(), which were a no-op.

Patch: clj-1793-3.patch

Screened by: Alex Miller

Environment

1.8.0-alpha2 - 1.8.0-alpha4

Attachments

3
  • 08 Sep 2016, 11:14 PM
  • 18 Aug 2015, 06:33 PM
  • 06 Aug 2015, 02:43 AM

Activity

Show:

Alex MillerMarch 14, 2017 at 5:42 PM

Looks like this one was missed when applying patches for release

Alex MillerSeptember 8, 2016 at 11:14 PM

Refreshed patch to apply to master. No semantic changes, attribution retained.

Nicola MomettoMarch 30, 2016 at 2:50 PM

The following code currently eventually causes an OOM to happen, the patch in this ticket correctly helps not holding onto the collection and doesn't cause memory to run infinitely

Before patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x))))) #'user/range* user=> (reduce + 0 (eduction (range* 0))) OutOfMemoryError Java heap space clojure.lang.RT.cons (RT.java:660)

After patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x))))) #'user/range* user=> (reduce + 0 (eduction (range* 0))) ;; runs infinitely without causing OOM

Nicola MomettoMarch 23, 2016 at 1:34 PM

Just a note that the original ticket for this issue had 10 votes

Ghadi ShaybanAugust 24, 2015 at 4:45 PM

Rich the original context is in CLJ-1250 which was a defect/problem. It was merged and revert because of a problem in the impl. This ticket is the continuation of the previous one, but unfortunately the title lost the context and became approach-oriented and not problem-oriented. Blame Alex. (I kid, it's an artifact of the mutable approach to issue management.)

Completed

Details

Assignee

Reporter

Labels

Approval

Ok

Patch

Code and Test

Priority

Affects versions

Fix versions

Created August 5, 2015 at 4:56 PM
Updated May 12, 2017 at 6:05 PM
Resolved May 12, 2017 at 6:05 PM

Flag notifications