<< Back to previous view

[LOGIC-50] Rel relation PersistentHashSet becomes LazySeq after issuing a retraction Created: 01/Sep/12  Updated: 28/Jul/13  Resolved: 05/Sep/12

Status: Closed
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: Text File core.logic-rel-001.patch     Text File core.logic-rel-002.patch    
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.





Generated at Wed Oct 01 07:23:53 CDT 2014 using JIRA 4.4#649-r158309.