Details

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.

Stuart Halloway
added a comment - 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.

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.

Paul Stadig
added a comment - 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.

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.

Paul Stadig
added a comment - 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.