Abstract over base type of Yi.Rope #15

ChrisPenner · 2017-09-14T01:56:57Z

Hello! This is in reference to #12

I'm opening the PR now even though there's a fair amount of work to do because I'd love to get some feedback as I go.

To give some background, the end goal is that I'd be able to store arbitrary annotations along-side my text in a still (relatively) performant fashion using a clean interface.

This PR will hope to accomplish that by effectively allowing the storage of any Segmented type in the rope. The segmented type needs to provide a base set of methods to manipulate it efficiently, as seen in the Segmented typeclass. I may end up pulling out the Segmented typeclass to a new package yet.

Anything that implements Segmented and Measure can be stored in a Braid which is an abstracted Rope.

An example use case here would be to keep track of all text that's selected or highlighted in a text editor using a bitmask. We want the selections/highlights to follow characters as they move around the screen, so we can store them alongside the text like so:

instance Segmented (V.Vector Bool) where -- Vector Bool is an unboxed bitmask vector
  type Segment (V.Vector Bool) = Bool
  head = V.head
  tail = V.tail
  -- etc

instance (Segmented a, Segmented b) => Segmented (a, b) where
  type Segment (a, b) = (Segment a, Segment b)
  head (a, b) = (head a, head b)
  tail (a, b) = (tail a, tail b)
  -- etc

Now that we've defined a segmentation, we can use Braid (Size, ()) (TX.Text, V.Vector Bool) to represent a composite of the two. We'd need to alter our measure-based functions slightly to allow them to dig inside the composite measure (which in this case would be a tuple of the two measures). But this is easily helped using a simple HasSize typeclass which we define for the tuple:

class HasSize m where
  getSize :: m -> Size

instance HasSize Size where
  getSize = id

instance HasSize (Size, a) where
  getSize = fst

Now so long as the typeclass is implemented we can mix and match any additional data with text and still get performant operations/measures.

A YiString is now simple a Braid Size TX.Text, I'll have to do some benchmarks, but it should compile to pretty much the same code as before so long as we make sure the compiler is inlining everything as it should.

Currently I've implemented the base abstraction in Yi.Braid and then re-export all the functions from Yi.Rope for back compatibility with the same types as they currently have. This should allow current users to be mostly unaffected. There's the one issue that I've renamed the constructor from YiString -> Braid, so that might need addressing.

Anyways, there's still tons of work to do and I need to update all the docs too. Let me know what you think of the general idea as I keep working on it. Cheers!

@Fuuzetsu

ChrisPenner · 2017-09-14T02:12:24Z

src/Yi/Rope.hs

 -- | Used to cache the size of the strings.
 data Size = Indices { charIndex :: {-# UNPACK #-} !Int
                      -- ^ How many characters under here?
                    , lineIndex :: Int
                      -- ^ How many lines under here?
                    } deriving (Eq, Show, Typeable)

+instance S.MSeg TX.Text where


Hoping to find a better way to do this, open to suggestions. The end user won't really need to care about it though.

ChrisPenner · 2017-09-14T02:13:41Z

src/Yi/Rope.hs

+instance S.MSeg TX.Text where
+  type Meas TX.Text = Size
+
+instance B.HasSize Size where


This typeclass would allow us to extract the size of a chunk's measure for custom measures.

ChrisPenner · 2017-09-14T02:50:39Z

src/Yi/Braid.hs

+{-# language FlexibleContexts #-}
+{-# language FlexibleInstances #-}
+{-# language TypeFamilies #-}
+{-# language UndecidableInstances #-}


This is unfortunate; it's required for the Eq, Ord, etc. instances. I don't really see why they're undecidable so it's possible I'm missing something and this could be removed somehow.

ghost · 2017-09-14T08:03:28Z

I like the idea. I think there should be a way to build Braid to Braid isomorphisms for transforming representations.

Fuuzetsu · 2017-09-14T08:30:43Z

I don't know when I'll get a chance to review this so please don't wait for my input.

Braid to Braid isomorphisms

an automorphism :)

ethercrow · 2017-09-14T16:36:20Z

stack.yaml

+  enable: true
+  packages:
+    - libcxx
+    - icu


we dropped icu dependency in 0.10

ethercrow · 2017-09-14T16:43:00Z

src/Yi/Braid.hs

+
+import qualified Yi.Segment as S
+
+type ValidBraid v a = (T.Measured v (Chunk a), S.Segmented a, HasSize v)


I took me a minute to realize this has kind Constraint, so could you add a kind signature?

ethercrow · 2017-09-14T16:44:48Z

Could you also fix comments in Braid module that talk about YiString?

ChrisPenner · 2017-09-16T18:43:43Z

In the most recent commit I pulled in the Data.ListLike dependency to replace my Segmented class, it has support for Text, Strings, Vectors, ByteStrings, etc. This allows the use of any of those types (and trivially any products or sums of those types) as a base type for a Braid. The implementation of ListLike for Text just relies on the underlying Data.Text functions so performance should be unaffected; with the exception of chunksOf and span which are unfortunately missing from the ListLike typeclass. I've implemented generic versions of these in Yi.Rope.Internal.ListLikeHelpers, but these are not specialized. Ideally we'd get these two methods into the ListLike class properly and could thus get more efficient implementations. This currently effects only toBraid and Yi.Braid.split and the Yi.Rope versions of the same. If performance here is an issue I can easily override the versions of these functions inside Yi.Rope to use the efficient Text versions.

This does pull in a pretty large amount of transitive dependencies which of course is not ideal. I'm open to suggestions, but I also believe this expands the utility of Braids pretty substantially. Let me know what you think of this idea in particular.

I've also gone through and cleaned up the docs.

ghost · 2017-09-16T18:49:32Z

Let me know what you think of this idea in particular.

That's related to issue #11 actually.

ChrisPenner · 2017-09-16T19:00:24Z

@siddhanathan Hrmm, Is that issue suggesting adding ListLike to YiString itself? I'd see that as a different (yet still related) issue. This is about utilizing ListLike to aid the underlying implementation.

Though the point definitely still stands that allowing any ListLike underlying representation could allow users to embed things in a Braid that perhaps they shouldn't, but in general I think the performance would still be better than any other possible option (even if the only benefit they get is use of the length tracking optimizations of Braid).

I see Braid as a useful base abstraction onto which you can build a few quick and easy optimizations. For instance with YiString it uses the base Braid abstraction but includes additional efficient combinators for working with newlines (which a Braid knows nothing about).

All that said, this does represent a pretty large change for Yi.Rope, I'd love to be able to work something out though. I think it opens up some pretty clever and interesting possibilities.

ChrisPenner · 2017-09-16T19:10:58Z

src/Yi/Braid.hs

+-- If however your function is unaffected by this 'chunking' behaviour
+-- you can tag or transform your underlying sequences or convert between
+-- `Braid` types.
+fmap' :: (ValidBraid v s a, ValidBraid q t b) => (a -> b) -> Braid v a -> Braid q b


Here's that Braid -> Braid isomorphism you wanted @siddhanathan

`withChunk` -> `fmap'` to allow converting between Braid types Map now allows changing the segment type

ChrisPenner · 2017-09-16T19:15:09Z

src/Yi/Braid.hs

+      Chunk _ x :< ts -> mkChunk LL.length (LL.filter p x) -| go ts
+
+-- | Maps the segments of the underlying chain.
+map :: (ValidBraid v s a, ValidBraid q t b) => (s -> t) -> Braid v a -> Braid q b


Also a generalized underlying segment map! @siddhanathan

v /= q would be an interesting case.

noughtmare · 2017-09-16T19:56:30Z

This does pull in a pretty large amount of transitive dependencies which of course is not ideal.

Yi already depends on ListLike, so it's not a problem for Yi.

ChrisPenner · 2017-09-16T21:47:10Z

Just ran the benchmarks and it looks like most operations have taken a hit; I'm really not very familiar with doing optimization work in Haskell so I'll need some tips from others to take care of that; my suspicions is that INLINE and SPECIALIZE pragmas can help us here. I was also forced to remove the UNPACK pragma on Chunk when I parametized it over the underlying type, so I figure that's probably got something to do with it. I haven't done high-performance haskell before though; I'll need some pointers to know where to start.

ChrisPenner · 2017-09-16T21:58:14Z

Another interesting possibility would be to move Chunk to Yi.Rope from Yi.Braid, write a ListLike instance for Chunk which exposes the cached length via LL.length, then use LL.length whenever needed in Yi.Braid instead of unpacking Chunks.

This should make the implementation simpler (less packing/unpacking) and would allow us to avoid wrapping other ListLike types in a redundant Chunk; e.g. Vectors already have V.length which is O(1), so the Chunk wrapper is unnecessary.

It would also allow us to specialize Chunk to Text values rather than being polymorphic which will let us UNPACK again.

Chunk is a Text specific optimization. This implements ListLike for Chunk and removes the need for a chunk in Braid.

ghost · 2017-09-18T13:31:32Z

@ChrisPenner I have some changes in https://github.com/siddhanathan/yi-rope/tree/abstract-base-type which I haven't tested thoroughly. I believe it should improve performance. See if it helps.

ChrisPenner · 2017-09-19T00:10:28Z

Unfortunately @siddhanathan it doesn't seem to have made much difference 😢

ghost · 2017-09-19T03:08:25Z

@ChrisPenner but it does make a small difference?

ChrisPenner · 2017-09-19T04:06:49Z

@siddhanathan Yes, but we're still orders of magnitude away. As per the conversation on Matrix; implementing a better LL.concat brought us in the ballpark, we should review the rest of the LL functions to find other improvements.

ChrisPenner · 2018-11-24T18:02:46Z

@ethercrow @noughtmare @Fuuzetsu I've been considering reviving this Pull Request for some new needs I have with Rasa; some options would include:

Leave Rope as an independent type and implementation and provide Braid alongside it as an orthoganal option, this avoids any performance degradation on Rope while still allowing people to use Braid when they like
clean it up against latest master and try to merge it in a similar form, this may cause some small performance degradation to Rope
fork the library into a new one just for the Braid type which would be maintained separately from Rope

Let me know if any of these seem reasonable 😄 Thanks!

Tagging @jmatsushita too

ChrisPenner added 4 commits September 12, 2017 20:21

Generalize YiString to Rope a

1f2a383

Clean up typeclasses

83d4f8b

Generalize Rope functions where possible

ab51bfe

Extract Braid.hs and use re-exports for pretty type sigs

c21dece

ChrisPenner commented Sep 14, 2017

View reviewed changes

ChrisPenner added 2 commits September 13, 2017 20:26

Clean up unused imports/pragmas

9efafd5

Use type variable instead of type family for getting Measure type

c3a890f

ChrisPenner commented Sep 14, 2017

View reviewed changes

Remove unnecessary extensions

2ea77af

ethercrow requested changes Sep 14, 2017

View reviewed changes

ChrisPenner added 2 commits September 16, 2017 12:02

Make docs generic for Yi.Braid

3e33ad8

Depend on ListLike for segmentation helpers

876bb5e

Docs cleanup

c35dac9

ChrisPenner added 2 commits September 16, 2017 12:51

Remove unused Segment.hs

d72f1e1

Remove impossible UNPACK pragma

1b48f12

ChrisPenner force-pushed the abstract-base-type branch from 2fbd4b2 to 260c3e0 Compare September 16, 2017 19:10

ChrisPenner commented Sep 16, 2017

View reviewed changes

Generalize withChunk and map

b2f7af5

`withChunk` -> `fmap'` to allow converting between Braid types Map now allows changing the segment type

ChrisPenner force-pushed the abstract-base-type branch from 260c3e0 to b2f7af5 Compare September 16, 2017 19:14

ChrisPenner commented Sep 16, 2017

View reviewed changes

Expose Yi.Braid publicly

d9a6bc9

Remove 'Chunk' from Braid

0c0735d

Chunk is a Text specific optimization. This implements ListLike for Chunk and removes the need for a chunk in Braid.

Add Inlinable Pragmas

d8fe019

ethercrow approved these changes Sep 19, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract over base type of Yi.Rope #15

Abstract over base type of Yi.Rope #15

ChrisPenner commented Sep 14, 2017 •

edited

Loading

ChrisPenner Sep 14, 2017

ChrisPenner Sep 14, 2017

ChrisPenner Sep 14, 2017

ghost commented Sep 14, 2017

Fuuzetsu commented Sep 14, 2017

ethercrow Sep 14, 2017

ethercrow Sep 14, 2017

ethercrow commented Sep 14, 2017

ChrisPenner commented Sep 16, 2017

ghost commented Sep 16, 2017

ChrisPenner commented Sep 16, 2017

ChrisPenner Sep 16, 2017

ChrisPenner Sep 16, 2017

ghost Sep 16, 2017

noughtmare commented Sep 16, 2017

ChrisPenner commented Sep 16, 2017 •

edited

Loading

ChrisPenner commented Sep 16, 2017 •

edited

Loading

ghost commented Sep 18, 2017

ChrisPenner commented Sep 19, 2017

ghost commented Sep 19, 2017

ChrisPenner commented Sep 19, 2017

ChrisPenner commented Nov 24, 2018


		import qualified Yi.Segment as S

		type ValidBraid v a = (T.Measured v (Chunk a), S.Segmented a, HasSize v)

Abstract over base type of Yi.Rope #15

Are you sure you want to change the base?

Abstract over base type of Yi.Rope #15

Conversation

ChrisPenner commented Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 14, 2017

Fuuzetsu commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethercrow commented Sep 14, 2017

ChrisPenner commented Sep 16, 2017

ghost commented Sep 16, 2017

ChrisPenner commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noughtmare commented Sep 16, 2017

ChrisPenner commented Sep 16, 2017 • edited Loading

ChrisPenner commented Sep 16, 2017 • edited Loading

ghost commented Sep 18, 2017

ChrisPenner commented Sep 19, 2017

ghost commented Sep 19, 2017

ChrisPenner commented Sep 19, 2017

ChrisPenner commented Nov 24, 2018

ChrisPenner commented Sep 14, 2017 •

edited

Loading

ChrisPenner commented Sep 16, 2017 •

edited

Loading

ChrisPenner commented Sep 16, 2017 •

edited

Loading