Welcome! Please see the About page for a little more info on how this works.

0 votes
in Clojure by

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.

8 Answers

0 votes
by

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.

0 votes
by

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.

0 votes
by

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.
0 votes
by
_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.
0 votes
by

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.

0 votes
by

Comment made by: alexmiller

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

0 votes
by

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.

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