<< Back to previous view

[CLJ-708] Multi-methods hold onto the head of their arguments Created: 08/Jan/11  Updated: 11/Mar/11  Resolved: 11/Mar/11

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2
Fix Version/s: Release 1.3

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: File muli-method-holds-head.diff     Text File multi-and-rest-fn-fix.patch     Text File multi-and-rest-fn-fix-unix.patch    
Patch: Code
Approval: Ok

 Description   

Multi-methods hold onto the head of their arguments when they are invoked. This means that if you invoke a multi-method with a lazy seq that cannot be held in memory all at once, you blow heap.

I'm not sure how best to write a test case for this particular issue, since the easiest way to test it is to run the JVM with a small heap and purposely evoke an OutOfMemoryError, so the attached patch has only the code changes. (However, I have verified the fix using a small heap.)

This will fix the issue for arities up to 6, for arities >=7 there is a bug in RestFn where it also holds the head of its arguments. If it is desirable to fix that bug as well, then I can submit a patch for it.



 Comments   
Comment by Paul Stadig [ 21/Jan/11 9:53 AM ]

Any thoughts on this?

Comment by Stuart Halloway [ 21/Jan/11 3:31 PM ]

I agree that no automated test is necessary for this. If you had attached your small-heap script I wouldn't have to write one from scratch to test it though.

Comment by Stuart Halloway [ 21/Jan/11 3:40 PM ]

Rich: two additional questions on this patch, other than just approval:

  1. Do we want another ticket to fix a similar issue in RestFn?
  2. Paul also inquired on the dev list about a 1.2.1 release for this fix. I dread the idea of going to point releases. Any suggestions on workarounds for this for people on the 1.2.x line?
Comment by Paul Stadig [ 21/Jan/11 4:17 PM ]

RE: a 1.2.1 release

This and the Keyword.intern fix both seem to be fixes for bugs that don't affect functionality, that would be worthy of a 1.2.1 release...I don't know if there are others that people would like to see in there.

They both also seem rather "trivial" to merge into 1.2. I'm willing to shepherd the release if no one else is interested in it.

Comment by Rich Hickey [ 26/Jan/11 6:54 AM ]

Patching both this and RestFn in a single go seems best.

Comment by Rich Hickey [ 26/Jan/11 6:55 AM ]

We should have a discussion about what might constitute a 1.2.1 on the dev list.

Comment by Paul Stadig [ 04/Feb/11 8:21 AM ]

Here is an updated patch for both multi-methods and RestFn.

I have also pushed a project to http://github.com/pjstadig/blow-heap that has some (gratuitous but automatically generated) tests for every arity combination.

There is a bin directory in that project that contains a blow-heap.sh file that can be run. It will call out to leiningen to run the tests, and the project.clj file is configured to start Java with a sufficiently small heap.

You can change the dependencies in the project.clj file to clojure 1.2.0, run the script, and see it fail. Then apply the patch to clojure, `mvn clean install`, go back to the blow-heap project and change the project.clj to use clojure 1.3.0 SNAPSHOT, `lein deps`, and rerun the tests to verify. (Whew!)

I believe this patch should also apply cleanly to the 1.2.0 branch, since the multi-method and rest-fn classes haven't changed since then, but if that's not the case I can send another rebased patch.

Comment by Stuart Halloway [ 22/Feb/11 7:58 PM ]

The second commit has the windows-line-ending problem. Anybody know how to fix this in an automated way?

Comment by Matthew Lee Hinman [ 22/Feb/11 11:45 PM ]

The same patch as Paul's, with unix line-endings

Comment by Matthew Lee Hinman [ 22/Feb/11 11:45 PM ]

Stuart: See attached patch run through the dos2unix tool

Comment by Stuart Halloway [ 06/Mar/11 1:18 PM ]

tests pass, approach looks fine. I didn't read every line, let mw know if there is some automated test or analysis you think I should do

Generated at Mon Oct 20 19:54:00 CDT 2014 using JIRA 4.4#649-r158309.