Clojure

Multi-methods hold onto the head of their arguments

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.2
  • Fix Version/s: Release 1.3
  • Component/s: None
  • Labels:
    None
  • 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.

Attachments

  1. File
    muli-method-holds-head.diff
    08/Jan/11 10:02 AM
    23 kB
    Paul Stadig
  2. Text File
    multi-and-rest-fn-fix.patch
    04/Feb/11 8:21 AM
    212 kB
    Paul Stadig
  3. Text File
    multi-and-rest-fn-fix-unix.patch
    22/Feb/11 11:45 PM
    208 kB
    Matthew Lee Hinman

Activity

Ascending order - Click to sort in descending order
Stuart Halloway made changes -
Field Original Value New Value
Assignee Stuart Halloway [ stu ]
Hide
Permalink
Paul Stadig added a comment -

Any thoughts on this?

Show
Paul Stadig added a comment - Any thoughts on this?
Hide
Permalink
Stuart Halloway added a comment -

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.

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

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?
Show
Stuart Halloway added a comment - 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?
Stuart Halloway made changes -
Fix Version/s Release.Next [ 10038 ]
Approval Screened
Hide
Permalink
Paul Stadig added a comment -

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.

Show
Paul Stadig added a comment - 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.
Hide
Permalink
Rich Hickey added a comment -

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

Show
Rich Hickey added a comment - Patching both this and RestFn in a single go seems best.
Hide
Permalink
Rich Hickey added a comment -

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

Show
Rich Hickey added a comment - We should have a discussion about what might constitute a 1.2.1 on the dev list.
Rich Hickey made changes -
Approval Screened Incomplete
Hide
Permalink
Paul Stadig added a comment -

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.

Show
Paul Stadig added a comment - 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.
Paul Stadig made changes -
Attachment multi-and-rest-fn-fix.patch [ 10106 ]
Hide
Permalink
Stuart Halloway added a comment -

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

Show
Stuart Halloway added a comment - The second commit has the windows-line-ending problem. Anybody know how to fix this in an automated way?
Stuart Halloway made changes -
Waiting On pjstadig
Hide
Permalink
Matthew Lee Hinman added a comment -

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

Show
Matthew Lee Hinman added a comment - The same patch as Paul's, with unix line-endings
Matthew Lee Hinman made changes -
Attachment multi-and-rest-fn-fix-unix.patch [ 10116 ]
Hide
Permalink
Matthew Lee Hinman added a comment -

Stuart: See attached patch run through the dos2unix tool

Show
Matthew Lee Hinman added a comment - Stuart: See attached patch run through the dos2unix tool
Stuart Halloway made changes -
Approval Incomplete Test
Waiting On pjstadig stu
Hide
Permalink
Stuart Halloway added a comment -

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

Show
Stuart Halloway added a comment - 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
Stuart Halloway made changes -
Approval Test Screened
Waiting On stu
Rich Hickey made changes -
Approval Screened Ok
Stuart Halloway made changes -
Status Open [ 1 ] Closed [ 6 ]
Resolution Completed [ 1 ]

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.