[CLJ-1793] Reducer instances hold onto the head of seqs Created: 05/Aug/15 Updated: 21/Sep/16
|Affects Version/s:||Release 1.8|
|Fix Version/s:||Release 1.9|
|Reporter:||Alex Miller||Assignee:||Alex Miller|
1.8.0-alpha2 - 1.8.0-alpha4
|Attachments:||0001-Clear-this-before-calls-in-tail-position.patch clj-1793-2.patch clj-1793-3.patch|
|Patch:||Code and Test|
(This ticket started life as
Problem: Original example was with reducers holding onto the head of a lazy seq:
Trickier example from
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
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.
Screened by: Alex Miller
|Comment by Alex Miller [ 05/Aug/15 12:16 PM ]|
The this ref is cleared prior to the println, but the next time through the while loop it needs the this ref to look up the closed over done field (via getfield).
Adding an additional check to the inTailCall() method to not include tail call in a loop addresses this case:
But want to check some more things before concluding that's all that's needed.
|Comment by Alex Miller [ 05/Aug/15 1:36 PM ]|
|Comment by Ghadi Shayban [ 05/Aug/15 3:12 PM ]|
Loop exit edges are erroneously being identified as places to clear 'this'. Only exits in the function itself or the outermost loop are safe places to clear.
|Comment by Ghadi Shayban [ 05/Aug/15 8:43 PM ]|
Patch addresses this bug and the regression in
See the commit message for an extensive-ish comment.
|Comment by Alex Miller [ 18/Aug/15 12:33 PM ]|
New patch is same as old, just adds jira id to beginning of commit message.
|Comment by Rich Hickey [ 24/Aug/15 10:00 AM ]|
Not doing this for 1.8, more thought needs to go into whether this is the right solution to the problem. And, what is the problem? This title of this patch is just something to do.
|Comment by Alex Miller [ 24/Aug/15 10:21 AM ]|
changing to vetted so this is at a valid place in the jira workflow
|Comment by Ghadi Shayban [ 24/Aug/15 10:45 AM ]|
Rich the original context is in
|Comment by Nicola Mometto [ 23/Mar/16 7:34 AM ]|
Just a note that the original ticket for this issue had 10 votes
|Comment by Nicola Mometto [ 30/Mar/16 8:50 AM ]|
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
|Comment by Alex Miller [ 08/Sep/16 5:14 PM ]|
Refreshed patch to apply to master. No semantic changes, attribution retained.