<< Back to previous view

[CLJS-849] Incorrect transients behavior (dissoc! deletes too much) Created: 02/Sep/14  Updated: 04/Sep/14  Resolved: 04/Sep/14

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Eldar Gabdullin Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: maps, transient

Attachments: Text File 0001-CLJS-849-fix-iteration-bound-in-pack-array-node.patch    

 Description   

The snippet below reproduces the problem

(defn bug [seq]
  (let [m (transient (zipmap seq (repeat 1)))]
    (loop [m m
           [x & rest] seq]
      (when x
        (if (contains? m x)
          (recur (dissoc! m x) rest)
          (throw (js/Error. "What's going on?")))))))

(bug [44 43 42 41 40 39 38 37 36 35 34 33 32 31 30 29 28 27 26 25 24]]


 Comments   
Comment by Eldar Gabdullin [ 02/Sep/14 12:46 PM ]

Ah, transient lookup is not supported in Clojure. Why it is allowed in ClojureScript, then?

Comment by Nicola Mometto [ 02/Sep/14 12:49 PM ]

The fact that contains? doesn't work in clojure is a bug, see http://dev.clojure.org/jira/browse/CLJ-700

Comment by Eldar Gabdullin [ 02/Sep/14 12:51 PM ]

Right, already spotted that.

Comment by Michał Marczyk [ 04/Sep/14 5:02 AM ]

Wow, this is a pretty serious bug – thanks!

It is caused by an incorrect iteration bound in pack-array-node. The attached patch fixes it.

Comment by Michał Marczyk [ 04/Sep/14 5:09 AM ]

Oops, forgot the test. Attaching the same fix + test case.

Comment by Michał Marczyk [ 04/Sep/14 5:31 AM ]

A final tweak.

Comment by David Nolen [ 04/Sep/14 7:31 AM ]

fixed https://github.com/clojure/clojurescript/commit/699b7487805c786b2eebe122ca3f09d7a5205c9e

Comment by Eldar Gabdullin [ 04/Sep/14 11:48 AM ]

Thank you.





[CLJ-1581] Inconsistent behavior in transient sets: they should allow contains? Created: 06/Nov/14  Updated: 06/Nov/14  Resolved: 06/Nov/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.8
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug, patch, transient

Attachments: Text File transient.patch    
Patch: Code and Test

 Description   

Transient maps and sets retain the behavior of persistent maps and sets.

When threading operations on transient sets, it is unfortunately impossible to test for membership, since the implementation of contains? defers to contains in clojure.lang.RT which does not

There are several solutions for this, I chose to extend contains in clojure.lang.RT to handle ITransientSet



 Comments   
Comment by Alex Miller [ 06/Nov/14 7:04 AM ]

Dupe of CLJ-700





[CLJ-1580] Transient collections should guarantee thread visibility Created: 05/Nov/14  Updated: 25/Nov/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transient

Approval: Incomplete

 Description   

With changes from CLJ-1498, transients are still thread isolated but may move between threads during their lifetime which introduces new concurrency concerns, namely visibility of changes across threads.






[CLJ-1498] Remove birth-thread check from transients Created: 08/Aug/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: collections, transient

Attachments: File clj-1498-2.diff     File clj-1498-3.diff     File clj-1498.diff    
Patch: Code
Approval: Ok

 Description   

Transients protect themselves from use by any thread other than the one that creates them. This is good for safety, however it eliminates certain valid usages of transients. For example, usage in a go-block might occur in subsequent invocations across multiple OS threads (but only one logical thread of control).

Current simple test:

user> (def v (transient []))
#'user/v
user> (persistent! @(future (conj! v 1)))
IllegalAccessError Transient used by non-owner thread  clojure.lang.PersistentVector$TransientVector.ensureEditable (PersistentVector.java:464)

Proposal: Remove the owner check from transient collections. (Leave the edit after persistent check as is.) The test above should succeed.

After:

user=> (def v (transient []))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

The clj-1498-3.diff version of the patch also replaces the AtomicReference<Thread> with AtomicBoolean as we can now track just ownership, not who owns it.

Doc update: Various pieces of documentation will need to be updated with this change, namely http://clojure.org/transients

Patch: clj-1498-3.diff

Alternative: Another idea would be to make this check optional with some kind of option on the transient call (transient coll :check-owner true). Not sure whether what the default would be for that.



 Comments   
Comment by Jozef Wagner [ 09/Aug/14 7:08 AM ]

I suggest to add a functionality to pass ownership of a transient to the different thread, or to release the ownership by passing nil.

user=> (def v (pass! (transient []) nil))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

pass! has to be called by current owner thread, or by any thread if the transient is currently released.

Comment by Alex Miller [ 13/Aug/14 1:42 PM ]

New patch that replaces AtomicReference<Thread> with AtomicBoolean.

Comment by Stuart Halloway [ 19/Aug/14 11:05 AM ]

Alex, can you please expand the example test you provided to a generative test that covers the following combinations:

  1. different collection sizes (above and below the ArrayMap size boundary)
  2. different shapes (vector vs. map)
  3. successful use across threads (positive use case this ticket enables)

data_structures.clj has helpers for generating transient interactions that you can build on.

Comment by Alex Miller [ 20/Aug/14 8:59 AM ]

Enhanced existing generative tests to test random actions against sets, vectors, and both PHM and PAM. Added additional actions to do transient modification actions in other threads as well as originating thread.





[CLJ-1416] Support transients in gvec Created: 06/May/14  Updated: 02/Oct/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: collections, transient

Attachments: Text File 0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch     Text File 0002-CLJ-1416-transients-hash-caching-interop-improvement.patch     Text File 0003-CLJ-1416-transients-hash-caching-interop-improvement.patch     Text File 0004-CLJ-1416-transients-hash-caching-interop-improvement.patch    
Patch: Code
Approval: Triaged

 Description   

Vectors of primitives produced by vector-of do not support transients.

core.rrb-vector implements transient support for vectors of primitives. Such transient-enabled vectors of primitives can be obtained in a number of ways: (1) using a gvec instance as an argument to fv/catvec (if RRB concatenation happens, which is not guaranteed) or fv/subvec; (2) passing a gvec instance to fv/vec, which as of core.rrb-vector 0.0.11 will simply rewrap the gvec tree in an RRB wrapper; (3) using fv/vector-of instead of clojure.core/vector-of. Native support in gvec would still be useful as part of an effort to make supported functionality consistent across vector flavours (see CLJ-787 in this connection); gvec is also simpler and still has (and is likely to maintain) a performance edge.

A port of core.rrb-vector's transient support to gvec is available here:

https://github.com/michalmarczyk/clojure/tree/transient-gvec

I'll bring it up to date with current master shortly.

See the clojure-dev thread for some benchmarks:

https://groups.google.com/d/msg/clojure-dev/9ozYI1e5SCM/BAIazVOkUmcJ



 Comments   
Comment by Michał Marczyk [ 13/May/14 5:32 AM ]

Here's the current version of the patch (0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch). It includes a few additional changes – here's the commit message:

CLJ-1416: transients, hash caching for gvec, Object methods for gvec seqs

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs

https://github.com/michalmarczyk/clojure/tree/transient-gvec-1.6

Comment by Michał Marczyk [ 05/Jul/14 2:59 AM ]

Here's an updated patch with some additional interop-related improvements.

The new commit message:

CLJ-1416: transients, hash caching, interop improvements for gvec

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs
  • Correctly implements iterator-related methods for gvec and gvec seqs
  • Introduces throw-unsupported and caching-hash (both marked private)
Comment by Andy Fingerhut [ 29/Aug/14 4:48 PM ]

Patch 0002-CLJ-1416-transients-hash-caching-interop-improvement.patch dated Jul 5 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Michał Marczyk [ 29/Aug/14 5:07 PM ]

Patch updated to apply cleanly to master.

Comment by Brandon Bloom [ 02/Oct/14 12:28 PM ]

Maybe this should be another ticket, but it would affect this patch, so I'll mention it here:

The ArrayManager interface is an incomplete abstraction. The original gvec code plus the new transients codepaths rely on System/arraycopy, rather than .arraycopy on the manager object. This means that it's impossible to create gvecs backed by non-JVM arrays. Or, in my case, to create a gvec of nibbles backed by an array of longs. See https://gist.github.com/brandonbloom/441a4b5712729dec7467

Comment by Brandon Bloom [ 02/Oct/14 1:34 PM ]

The current patch has a bug on line 762:

(let [node ^clojure.core.VecNode (.ensureEditable this node)

There is no such signature, only these:

(ensureEditable [this]
(ensureEditable [this node shift]

I discovered this problem using https://github.com/ztellman/collection-check

Comment by Michał Marczyk [ 02/Oct/14 2:46 PM ]

Thanks for the catch! Fixed patch attached. (There was in fact one more bug in editableArrayFor, also fixed in this version.)

Comment by Michał Marczyk [ 02/Oct/14 2:57 PM ]

As for gvecs of nibbles, could that be a separate ticket with patches building on top of this one?

On a separate note, core.rrb-vector could support vectors of nibbles as an extra feature (and adopt built-in gvec's representation if indeed the built-in gvec comes to support this feature at some point). Do you think that'd be useful?

Comment by Michał Marczyk [ 02/Oct/14 3:01 PM ]

Of course vectors of nibbles could be implemented today with a separate vector type wrapping a gvec of longs, but the implementation would be more involved. I wonder what kind of performance difference there would be between the wrapper approach and the "nibble AM" approach…





[CLJ-1386] Add transient? predicate Created: 17/Mar/14  Updated: 31/Aug/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0001-Add-transient-predicate.patch     Text File 0002-Add-transient-predicate.patch     Text File 0003-Add-transient-predicate.patch     Text File 0004-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.

Comment by Andy Fingerhut [ 06/Aug/14 2:16 PM ]

Patch 0002-Add-transient-predicate.patch dated Mar 22 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether this patch is straightforward to update.

Comment by Devin Walters [ 06/Aug/14 4:11 PM ]

I've updated the patch to 0003-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.

Comment by Andy Fingerhut [ 29/Aug/14 4:44 PM ]

Patch 0003-Add-transient-predicate.patch dated Aug 6 2013 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Comment by Devin Walters [ 31/Aug/14 12:01 AM ]

I've updated the patch to 0004-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.





[CLJ-1285] Persistent assoc/conj on a transient-created collision node Created: 28/Oct/13  Updated: 11/Nov/13  Resolved: 11/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: transient

Attachments: File persistent-assoc-after-collision.diff     File transient-generative-test.diff    
Patch: Code and Test
Approval: Ok

 Description   

Bug reported by Zach Tellman https://groups.google.com/d/msg/clojure-dev/HvppNjEH5Qc/1wZ-6qE7nWgJ

Since transients were introduced the invariant array.length == count*2 doesn't hold for HashCollisionNode.
However persistent .without still relies on it.

Hence persistent dissoc on a collision node created by transients fails.

(let [a (reify Object (hashCode [_] 42))
      b (reify Object (hashCode [_] 42))]
      (= (-> #{a b} transient (disj! a) persistent! (conj a))
       (-> #{a b} transient (disj! a) persistent! (conj a))))

returns false.

Patch: persistent-assoc-after-collision.diff

Generative test patch: transient-generative-test.diff

The generative test reliably reproduces the error. It is simpler than the original test that found the bug but tests a series conj/disj/transient/persistent actions on a set. I've included it separately in case we decide not to apply.

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 29/Oct/13 9:58 PM ]

I can confirm that the patch works for me. As per our #clojure conversation, I've done the ClojureScript port; see CLJ-648.

Comment by Reid Draper [ 29/Oct/13 11:28 PM ]

I've run Zach's original test, as well as my own simple-check test. Both are passing.

Comment by Alex Miller [ 30/Oct/13 9:33 AM ]

I don't suppose we could get a generative test (prob need to use test.generative which is already included) to test this stuff similar to the original test that found the bug?

Very much hoping to get this into 1.6.

Comment by Andy Fingerhut [ 31/Oct/13 10:52 AM ]

Alex, I suspect clojure-dev would reach a much wider audience for your request than a comment on this ticket, which only has 3 watchers, and I don't think many people besides you and I watch the stream of all ticket state changes as they go by.

Comment by Michał Marczyk [ 01/Nov/13 5:19 AM ]

Just wanted to note that this patch, apart from preventing the hash-based collections from failing Zach's test suite, also makes avl.clj collections pass (now that I've released fixes for the two bugs uncovered by the test suite in avl.clj 0.0.9). This provides some cross-validation, I think.

(The built-in sorted collections pass either way, because they don't support transient ops.)

Also, David Nolen has merged the ClojureScript port of the patch.

Comment by Alex Miller [ 01/Nov/13 7:35 AM ]

I'm going to take a crack at repro with test.generative this morning - wish me luck!

Comment by Alex Miller [ 03/Nov/13 10:40 PM ]

Added a simplified version of a test-generative test and marked screened.

Comment by Alex Miller [ 11/Nov/13 11:17 AM ]

Patch was applied to master for 1.6.





Generated at Thu Nov 27 04:39:49 CST 2014 using JIRA 4.4#649-r158309.