Swapping IO for abstract IO type-classes #396

recursion-ninja · 2024-09-21T22:01:17Z

Refactor to use `io-classes` type-classes instead of `IO`

A reasonably large-scale refactoring (of API type signatures) which addresses the numerous TODO annotations stating "replace by io-classes constraints for IO simulation" in order to provide a more general, polymorphic API for users of the lsm-tree library. The indirect object to be replaced that the TODO annotation references are the m ~ IO constraints found on the same line(s) prefixing the TODO annotation, allowing the post-refactoring functions to abstract over IO operations rather than exposing an API which is specialized to IO. While most TODO annotations were in the Database.LSMTree.Internal module, this PR touches many additional modules. This wide footprint of changes is necessary because the Database.LSMTree.Internal module imports many other modules from the lsm-tree project, hence each of these "dependency modules" required a "IO-abstraction" API update in order to facilitate the "IO-abstraction" of the Database.LSMTree.Internal module.

The io-classes which are utilized in place of specialized IO functions are:

Important Note

Wherever the PR updated a module API from exposing a specialized IO version of a function to a generalized io-classes function, a SPECIALISE directive was also added which specializes the polymorphic m type parameter to IO. The rational here is to have the module's interface file still expose a version of the function which is specialized to IO in order to prevent any performance regressions. However, this may be excessive and result in unnecessarily longer compiler times and code sizes.

I would like some commentary from reviewers more experience balancing SPECIALISE directive usage as to whether the abundant and boilerplate additions of theSPECIALISE directives are a wise decision.

jorisdral

I would like some commentary from reviewers more experience balancing SPECIALISE directive usage as to whether the abundant and boilerplate additions of the SPECIALISE directives are a wise decision.

Generalising the code to work for any IO-like monad is done for testing purposes only, and since the library is intended to be highly performant, the generalisation should ideally have no overhead. So, the SPECIALISE pragmas are warranted -- the compile time is also not prohibitively long at the moment, even with the changes in this PR

On that note, we should verify that the generalisation does not impact our performance. Let's run the benchmarks to see if we can see a clear difference

Also, I have some proposed changes on the branch jdral/io-classes-for-abstract-types. You can cherry-pick what you want to include.

It's puzzling that tests are currently timing out -- there are no logic changes AFAICS. We should find out what is happening here

Note that with the changes on my branch, it seems most of our tests succeed that are currently failing, except for propLockstepIO_RealImpl_RealFS. However, I'm hesitant to say that my changes "fix" the problem if we don't know the origin of the bug. However, I've played around a bit with the statemachine tests, and it seems that disabling the Open action makes the propLockstepIO_RealImpl_RealFS pass... Not sure why

jorisdral · 2024-09-24T07:25:50Z

src/Database/LSMTree/Internal.hs

+{-# INLINE withOpenSession #-}
+{-# SPECIALISE withOpenSession :: Session IO h -> (SessionEnv IO h -> IO a) -> IO a #-}


With an INLINE, the SPECIALISE is probably not necessary, but it doesn't hurt to include it anyway

jorisdral · 2024-09-24T07:31:04Z

src/Database/LSMTree/Internal/Run.hs

+openFromDisk ::
+  forall m h.
+     (MonadFix m, MonadSTM m, MonadThrow m, PrimMonad m)
+  => HasFS m h


Suggested change

openFromDisk ::

forall m h.

(MonadFix m, MonadSTM m, MonadThrow m, PrimMonad m)

=> HasFS m h

openFromDisk ::

forall m h.

(MonadFix m, MonadSTM m, MonadThrow m, PrimMonad m)

=> HasFS m h

jorisdral · 2024-09-24T07:31:37Z

src/Database/LSMTree/Internal/RunBuilder.hs

 import           Control.Monad.Primitive
-import qualified Control.Monad.ST as ST
+--import qualified Control.Monad.ST as ST


Suggested change

--import qualified Control.Monad.ST as ST

jorisdral · 2024-09-24T07:33:32Z

src/Database/LSMTree/Internal/RunBuilder.hs

@@ -140,18 +150,20 @@ addLargeSerialisedKeyOp fs builder@RunBuilder{runBuilderAcc} key page overflowPa
    writeRawOverflowPages fs builder overflowPages'
    for_ chunks $ writeIndexChunk fs builder

+{-# SPECIALISE unsafeFinalise :: HasFS IO h -> HasBlockIO IO h -> Bool -> RunBuilder (PrimState IO) (FS.Handle h) -> IO (RunFsPaths, Bloom SerialisedKey, IndexCompact, NumEntries) #-}


Use RealWorld instead of PrimState IO.

Suggested change

{-# SPECIALISE unsafeFinalise :: HasFS IO h -> HasBlockIO IO h -> Bool -> RunBuilder (PrimState IO) (FS.Handle h) -> IO (RunFsPaths, Bloom SerialisedKey, IndexCompact, NumEntries) #-}

{-# SPECIALISE unsafeFinalise :: HasFS IO h -> HasBlockIO IO h -> Bool -> RunBuilder RealWorld (FS.Handle h) -> IO (RunFsPaths, Bloom SerialisedKey, IndexCompact, NumEntries) #-}

Make sure to change every other occurrence in this PR as well

jorisdral · 2024-09-24T07:34:00Z

src/Database/LSMTree/Internal/RunBuilder.hs

 -- | Close a run that is being constructed (has not been finalised yet),
 -- removing all files associated with it from disk.
 -- After calling this operation, the run must not be used anymore.
 --
 -- TODO: Ensure proper cleanup even in presence of exceptions.
-close :: HasFS IO h -> RunBuilder RealWorld (FS.Handle h) -> IO ()
+close :: (MonadSTM m) => HasFS m h -> RunBuilder (PrimState m) (FS.Handle h) -> m ()


Suggested change

close :: (MonadSTM m) => HasFS m h -> RunBuilder (PrimState m) (FS.Handle h) -> m ()

close :: MonadSTM m => HasFS m h -> RunBuilder (PrimState m) (FS.Handle h) -> m ()

jorisdral · 2024-09-24T07:34:45Z

src/Database/LSMTree/Internal/RunBuilder.hs

  Helpers
 -------------------------------------------------------------------------------}

-writeRawPage :: HasFS IO h -> RunBuilder RealWorld (FS.Handle h) -> RawPage -> IO ()
+{-# SPECIALISE writeRawPage :: HasFS IO h -> RunBuilder (PrimState IO) (FS.Handle h) -> RawPage -> IO () #-}
+writeRawPage :: (MonadSTM m, PrimMonad m) => HasFS m h -> RunBuilder (PrimState m) (FS.Handle h) -> RawPage -> m ()


Let's wrap the function signatures for helpers in this section over multiple lines if they far exceed 80 characters in width

jorisdral · 2024-09-24T07:35:44Z

src/Database/LSMTree/Internal/RunReader.hs

@@ -52,7 +56,7 @@ import           System.FS.BlockIO.API (HasBlockIO)
 data RunReader m fhandle = RunReader {
      -- | The disk page currently being read. If it is 'Nothing', the reader
      -- is considered closed.
-      readerCurrentPage    :: !(IORef (Maybe RawPage))
+      readerCurrentPage    :: !(STRef (PrimState m) (Maybe RawPage))


Try a MutVar instead

jorisdral · 2024-09-24T07:36:45Z

src/Database/LSMTree/Internal/RunReader.hs

 -- | Returns 'Nothing' on EOF.
-readDiskPage :: HasFS IO h -> FS.Handle h -> IO (Maybe RawPage)
+readDiskPage :: (MonadCatch m, MonadSTM m, PrimMonad m) => HasFS m h -> FS.Handle h -> m (Maybe RawPage)


Suggested change

readDiskPage :: (MonadCatch m, MonadSTM m, PrimMonad m) => HasFS m h -> FS.Handle h -> m (Maybe RawPage)

readDiskPage :: (MonadCatch m, PrimMonad m) => HasFS m h -> FS.Handle h -> m (Maybe RawPage)

recursion-ninja requested review from dcoutts, jorisdral and mheinzel as code owners September 21, 2024 22:01

Swapping IO for abstract IO type-classes

c0c4f8c

jorisdral requested changes Sep 24, 2024

View reviewed changes

Opting out of Snapshot operation in statemachine

52875ff

recursion-ninja force-pushed the recursion-ninja/io-classes-for-abstract-types branch from 00ddaee to 52875ff Compare September 25, 2024 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swapping IO for abstract IO type-classes #396

Swapping IO for abstract IO type-classes #396

recursion-ninja commented Sep 21, 2024

jorisdral left a comment

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

jorisdral Sep 24, 2024

		{-# INLINE withOpenSession #-}
		{-# SPECIALISE withOpenSession :: Session IO h -> (SessionEnv IO h -> IO a) -> IO a #-}

	{-# SPECIALISE unsafeFinalise :: HasFS IO h -> HasBlockIO IO h -> Bool -> RunBuilder (PrimState IO) (FS.Handle h) -> IO (RunFsPaths, Bloom SerialisedKey, IndexCompact, NumEntries) #-}
	{-# SPECIALISE unsafeFinalise :: HasFS IO h -> HasBlockIO IO h -> Bool -> RunBuilder RealWorld (FS.Handle h) -> IO (RunFsPaths, Bloom SerialisedKey, IndexCompact, NumEntries) #-}

	close :: (MonadSTM m) => HasFS m h -> RunBuilder (PrimState m) (FS.Handle h) -> m ()
	close :: MonadSTM m => HasFS m h -> RunBuilder (PrimState m) (FS.Handle h) -> m ()

	readDiskPage :: (MonadCatch m, MonadSTM m, PrimMonad m) => HasFS m h -> FS.Handle h -> m (Maybe RawPage)
	readDiskPage :: (MonadCatch m, PrimMonad m) => HasFS m h -> FS.Handle h -> m (Maybe RawPage)

Swapping IO for abstract IO type-classes #396

Are you sure you want to change the base?

Swapping IO for abstract IO type-classes #396

Conversation

recursion-ninja commented Sep 21, 2024

Refactor to use io-classes type-classes instead of IO

Important Note

jorisdral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor to use `io-classes` type-classes instead of `IO`