transient blows up when passed a vector created by subvec

Question

Subvectors created with subvec from a PersistentVector cannot be made transient:

user=> (transient (subvec [1 2 3 4] 2)) ClassCastException clojure.lang.APersistentVector$SubVector cannot be cast to clojure.lang.IEditableCollection clojure.core/transient (core.clj:2864)

Cause: {{APersistentVector$SubVector}} does not implement {{IEditableCollection}}

Patch: CLJ-787-p1.patch

Approach: Create a {{TransientSubVector}} based on an underlying {{TransientVector}}.

Two assumptions:
It's okay for {{TransientSubVector}} to delegate the {{ensureEditable}} functionality to the underlying {{TransientVector}} (sometimes explicitly, sometimes implicitly) - calling {{ensureEditable}} explicitly also requires that the field for the underlying vector be the concrete {{TransientVector}} type rather than the {{ITransientVector}} interface.
When an operation that would throw an exception on a {{PersistentVector}} happens from the wrong thread (or after persistent!), we throw that exception rather than the {{IllegalAccessError}} that transients throw when accessed inappropriately.

Answer 1

Comment made by: stuart.sierra

Confirmed. APersistentVector$SubVector does not implement IEditableCollection.

The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.

Answer 2

Comment made by: gfredericks

We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?

Preparing a patch to that effect.

Answer 3

Comment made by: gfredericks

Text from the commit msg:

Made two assumptions:

• It's okay for TransientSubVector to delegate the ensureEditable
  functionality to the underlying TransientVector (sometimes
  explicitely, sometimes implicitely) -- calling ensureEditable
  explicitely also requires that the field for the underlying vector
  be the concrete TransientVector type rather than the
  ITransientVector interface.
• When an operation that would throw an exception on a
  PersistentVector happens from the wrong thread (or after
  persistent!), we throw that exception rather than the
  IllegalAccessError that transients throw when accessed
  inappropriately.

Answer 4

_Comment made by: alexmiller_

I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.

A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):


user=> (transient (subvec (first {:a 1}) 0 1))
ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IEditableCollection  clojure.lang.APersistentVector$TransientSubVector.<init> (APersistentVector.java:592)


PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too.  TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.  

I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).

Needs more thought.

Answer 5

Comment made by: jafingerhut

Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.

Answer 6

Comment made by: alexmiller

No good solution to consider right now, removing from 1.6.

Answer 7

Comment made by: alexmiller

Rich moved this to vetted, but I think it should have been left as Incomplete as last comments were never addressed.

Answer 8

Reference: https://clojure.atlassian.net/browse/CLJ-787 (reported by aredington)