[LOGIC-50] Rel relation PersistentHashSet becomes LazySeq after issuing a retraction Created: 01/Sep/12 Updated: 05/Sep/12 Resolved: 05/Sep/12 |
|
| Status: | Resolved |
| Project: | core.logic |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Defect | Priority: | Minor |
| Reporter: | Aaron Brooks | Assignee: | David Nolen |
| Resolution: | Completed | Votes: | 0 |
| Labels: | performance | ||
| Attachments: |
|
| Patch: | Code |
| Description |
|
The first retraction of facts from a relation transforms the internal PersistentHashSet -set var+atom, which guarantees uniqueness, into a LazySeq which allows duplicate facts to be added. Once the first retraction occurs, a LazySeq persists for the rest of the life of the relation. Only the value of the primary -set var+atom is affected, not the -index var+atom values. This issue is not an external correctness issue but does affect performance of subsequent duplicate fact additions which grow the size relation. Preparation: user=> (defrel foo x y)
#<user$eval4287$fn__4288 user$eval4287$fn__4288@1a9d489b>
user=> (facts foo [[:joe :man][:jane :woman][:sue :boy]])
nil
user=> foo_2-set
#<Atom@52aaf223: #{[:joe :man] [:jane :woman] [:sue :boy]}>
user=> (class @foo_2-set)
clojure.lang.PersistentHashSet
user=> (retraction foo :jane :woman)
nil
Without patch applied: user=> foo_2-set #<Atom@52aaf223: ([:joe :man] [:sue :boy])> user=> (class @foo_2-set) clojure.lang.LazySeq user=> (facts foo [[:joe :man][:jane :woman][:sue :boy]]) nil user=> foo_2-set #<Atom@52aaf223: ([:sue :boy] [:jane :woman] [:joe :man] [:joe :man] [:sue :boy])> user=> (class @foo_2-set) clojure.lang.LazySeq With patch applied: user=> foo_2-set
#<Atom@31eb9b15: #{[:joe :man] [:sue :boy]}>
user=> (class @foo_2-set)
clojure.lang.PersistentHashSet
user=> (facts foo [[:joe :man][:jane :woman][:sue :boy]])
nil
user=> foo_2-set
#<Atom@31eb9b15: #{[:joe :man] [:jane :woman] [:sue :boy]}>
user=> (class @foo_2-set)
clojure.lang.PersistentHashSet
I've filed this as a Minor issue as it does not affect core.logic correctness and degraded performance can be avoided by not re-asserting duplicate facts. I will also issue a GitHub pull request which can be used or ignored at your convenience. |
| Comments |
| Comment by David Nolen [ 02/Sep/12 5:45 PM ] |
|
Thanks! Is this meant to be applied to master? |
| Comment by Aaron Brooks [ 03/Sep/12 6:56 PM ] |
|
(cf. here and GitHub I'll keep this thread on JIRA.) Yes, this is targeted for master. I don't know if this warrants a release unto itself. |
| Comment by Aaron Brooks [ 04/Sep/12 9:17 AM ] |
|
I forgot to mention that I needed to add "src/test/clojure" to the project.clj :source-paths vector to get lein1/2's test command to run the tests. Is that expected? Is there another way to run the tests that I'm missing? I'm happy to file a separate ticket/patch to address this if project.clj needs to be modified. |
| Comment by David Nolen [ 04/Sep/12 9:25 AM ] |
|
Thanks for the information. Will apply to master. I actually run tests with "mvn test", I've updated :source-paths so other people can run the tests with lein. |
| Comment by David Nolen [ 04/Sep/12 9:26 AM ] |
|
I tried to apply the patch but I'm getting a "Patch format detection failed". |
| Comment by Aaron Brooks [ 04/Sep/12 4:16 PM ] |
|
The attached patch should work fine with git-apply (in either "cat foo.patch |git apply" or "git apply foo.patch" form). I made the GitHub pull request as I figured that would be the easiest path to pull the changes in. |
| Comment by David Nolen [ 04/Sep/12 6:04 PM ] |
|
Yes the patch is not properly formatted and we're not supposed to take pull requests. The patch is missing attribution info. I normally apply patches with "git am foo.patch". I used the following guide for figuring out how to generate patches that can be applied with "git am", http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git. |
| Comment by Aaron Brooks [ 05/Sep/12 9:25 AM ] |
|
My apologies for the extra run-around here. I've attached an -002 version of the patch created with git-format-patch which should be amenable to git-am. |
| Comment by David Nolen [ 05/Sep/12 11:32 AM ] |
|
fixed, http://github.com/clojure/core.logic/commit/9bc6eb42be28bfd2b493657344f6eea8f5ed657c. pushing out a 0.8.0-alpha3 release as well. |