Resolving TODO: PrimVar for UniqCounter #508

recursion-ninja · 2024-12-22T18:56:41Z

Description

Updating the mutable pointer type of UniqCounter to be a PrimVar rather than a StrictMVar.

src/Database/LSMTree/Internal/UniqCounter.hs

jorisdral · 2024-12-23T12:36:28Z

src/Database/LSMTree/Internal/UniqCounter.hs

--
-- TODO: could we use a PrimVar here?
-newtype UniqCounter m = UniqCounter (StrictMVar m Word64)
+newtype UniqCounter m = UniqCounter (PrimVar (PrimState m) Word64)


Suggested change

newtype UniqCounter m = UniqCounter (PrimVar (PrimState m) Word64)

newtype UniqCounter s = UniqCounter (PrimVar s Word64)

Changing (PrimState m) to s here causes a lot of "upstream" breakages in the type-class constraint which call functions that interact with UniqCounter. Generally it requires creating a new type variable s in addition to the existing m, and adding PrimState m ~ s in a lot of places.

Is there a technical reason to make this change? Performance or memory implications I might not be aware of?

It's not incorrect, it's just more restrictive to require m instead of s. We should add at least a TODO to change it, even if it would cause code changes elsewhere

jorisdral · 2024-12-31T11:07:23Z

src/Database/LSMTree/Internal/UniqCounter.hs

+
+-- |
+-- This call to @unsafeCoerce@ relies on the machine's binary representation of
+-- the @Int@ type to be safe. The binary representation must be 64-bots wide, a


Suggested change

-- the @Int@ type to be safe. The binary representation must be 64-bots wide, a

-- the @Int@ type to be safe. The binary representation must be 64-bits wide, a

jorisdral · 2024-12-31T11:12:23Z

src/Database/LSMTree/Internal/UniqCounter.hs

--
-- TODO: could we use a PrimVar here?
-newtype UniqCounter m = UniqCounter (StrictMVar m Word64)
+newtype UniqCounter m = UniqCounter (PrimVar (PrimState m) Word64)


It's not incorrect, it's just more restrictive to require m instead of s. We should add at least a TODO to change it, even if it would cause code changes elsewhere

jorisdral · 2024-12-31T11:19:21Z

src/Database/LSMTree/Internal/UniqCounter.hs

-newtype UniqCounter m = UniqCounter (StrictMVar m Word64)
+-- Additionally, it is safe to @unsafeCoerce@ between `Int` and `Word64` for
+-- the same reason, the use must guarantee a 2's compliment representation for
+-- signed integral types. See thethis module's @convert@ function for more info.


Suggested change

-- signed integral types. See thethis module's @convert@ function for more info.

-- signed integral types. See this module's @convert@ function for more info.

jorisdral · 2024-12-31T11:20:25Z

src/Database/LSMTree/Internal/UniqCounter.hs

+-- |
+-- This call to @unsafeCoerce@ relies on the machine's binary representation of


Suggested change

-- |

-- This call to @unsafeCoerce@ relies on the machine's binary representation of

-- | This call to @unsafeCoerce@ relies on the machine's binary representation of

jorisdral · 2024-12-31T11:21:03Z

src/Database/LSMTree/Internal/UniqCounter.hs

-- TODO: could we use a PrimVar here?
-newtype UniqCounter m = UniqCounter (StrictMVar m Word64)
+-- Additionally, it is safe to @unsafeCoerce@ between `Int` and `Word64` for
+-- the same reason, the use must guarantee a 2's compliment representation for


Suggested change

-- the same reason, the use must guarantee a 2's compliment representation for

-- the same reason, the use must guarantee a 2's complement representation for

jorisdral · 2024-12-31T11:21:14Z

src/Database/LSMTree/Internal/UniqCounter.hs

+-- the @Int@ type to be safe. The binary representation must be 64-bots wide, a
+-- given guarantee by the user according to the @README.md@. Additionally, the
+-- binary representation of @Int@ must form an additive ring which is homomorphic
+-- with @Word64@ (such as 2's compliment); i.e.:


Suggested change

-- with @Word64@ (such as 2's compliment); i.e.:

-- with @Word64@ (such as 2's complement); i.e.:

jorisdral · 2024-12-31T11:22:35Z

src/Database/LSMTree/Internal/UniqCounter.hs

+-- @
+-- let limit = maxBound :: Int
+-- in  forall (x :: Int).
+--       (unsafeCoerce limit :: Word64) + (unsafeCoerce x :: Word64) === unsafeCoerce (limit + x)
+-- @


Let's make this into a test!

jorisdral · 2024-12-31T11:24:07Z

src/Database/LSMTree/Internal/UniqCounter.hs

-- TODO: could we use a PrimVar here?
-newtype UniqCounter m = UniqCounter (StrictMVar m Word64)
+-- Additionally, it is safe to @unsafeCoerce@ between `Int` and `Word64` for
+-- the same reason, the use must guarantee a 2's compliment representation for


Are there cases where ints are not represented as 2's complement? Does it depend on GHC, gcc, the OS, the machine architecture?

dcoutts

I don't think there's any strong reason why the counter should be represented by a Word64 or an Int. So I don't think we need to get into the weeds about unsafe conversions. We can both UniqueCounter and Unique as Int and convert to RunNumber safely.

Similarly, I'm not sure there's any strong reason that RunNumber needs to be Word64 rather than Int.

dcoutts · 2025-01-06T16:03:15Z

src/Database/LSMTree/Internal/UniqCounter.hs

+-- @
+{-# INLINE convert #-}
+convert :: Int -> Word64
+convert = unsafeCoerce


I don't see why we need to unsafely coerce. An ordinary conversion should be just fine, no?

dcoutts · 2025-01-07T10:59:48Z

I'll take this one over since @recursion-ninja is away. I'm going to simplify to use Int everywhere.

jorisdral · 2025-01-07T14:11:37Z

Similarly, I'm not sure there's any strong reason that RunNumber needs to be Word64 rather than Int.

We only use RunNumber as the name for runs, and I figured it would be nicest to have only positive integers as names

dcoutts · 2025-01-07T14:32:05Z

We only use RunNumber as the name for runs, and I figured it would be nicest to have only positive integers as names

We still can. We initialise the unique counter at 0, so they'll all be positive.

dcoutts

LGTM

jorisdral · 2025-01-07T14:39:21Z

We only use RunNumber as the name for runs, and I figured it would be nicest to have only positive integers as names

We still can. We initialise the unique counter at 0, so they'll all be positive.

I was thinking about when the counter would wrap around from maxBound to minBound when we increment it. It's only going to happen after a verrrry long time so it's unlikely any cardano node would ever reach that point, but I also don't see it costing us much to use Word64. The run number / table ID / cursor ID are only used as names after all, not to compute anything with

dcoutts · 2025-01-07T15:00:41Z

It's only going to happen after a verrrry long time so it's unlikely any cardano node would ever reach that point

It certainly would not happen for a Cardano node since those only run on 64bit platforms.

And for any other use case on a 32bit platform, it would only happen after 2 billion table duplications within the same session. At which point the bigger problem would be the clashes not the negative number.

RunNumber is rather unnecessarily a Word64 when Int would do just fine and involve fewer conversions. Also add TableId and CursorId newtypes (rather than raw Word64) and make these use Int too for the same reasons. Add and use direct conversion functions uniqueTo{Table,Cursor}Id.

recursion-ninja requested review from dcoutts, jorisdral, mheinzel and wenkokke as code owners December 22, 2024 18:56

jorisdral reviewed Dec 23, 2024

View reviewed changes

jorisdral mentioned this pull request Dec 23, 2024

Resolving TODO: Converted FilePointer to Word representation #509

Merged

recursion-ninja force-pushed the recursion-ninja/UniqCounter-primitive branch from bc26381 to 6387479 Compare December 30, 2024 16:51

jorisdral reviewed Dec 31, 2024

View reviewed changes

dcoutts reviewed Jan 6, 2025

View reviewed changes

dcoutts force-pushed the recursion-ninja/UniqCounter-primitive branch from 6387479 to d406ff0 Compare January 7, 2025 14:30

dcoutts approved these changes Jan 7, 2025

View reviewed changes

dcoutts force-pushed the recursion-ninja/UniqCounter-primitive branch from d406ff0 to 6fa48f1 Compare January 7, 2025 14:57

recursion-ninja and others added 2 commits January 8, 2025 00:04

Resolving TODO: PrimVar for UniqCounter

e3b9540

dcoutts force-pushed the recursion-ninja/UniqCounter-primitive branch from 6fa48f1 to 4dcde00 Compare January 8, 2025 00:06

dcoutts enabled auto-merge January 8, 2025 00:06

dcoutts added this pull request to the merge queue Jan 8, 2025

Merged via the queue into main with commit 39343e5 Jan 8, 2025
27 checks passed

dcoutts deleted the recursion-ninja/UniqCounter-primitive branch January 8, 2025 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolving TODO: PrimVar for UniqCounter #508

Resolving TODO: PrimVar for UniqCounter #508

recursion-ninja commented Dec 22, 2024

jorisdral Dec 23, 2024

recursion-ninja Dec 30, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

jorisdral Dec 31, 2024

dcoutts left a comment

dcoutts Jan 6, 2025

dcoutts commented Jan 7, 2025

jorisdral commented Jan 7, 2025

dcoutts commented Jan 7, 2025

dcoutts left a comment

jorisdral commented Jan 7, 2025

dcoutts commented Jan 7, 2025

	newtype UniqCounter m = UniqCounter (PrimVar (PrimState m) Word64)
	newtype UniqCounter s = UniqCounter (PrimVar s Word64)

	-- the @Int@ type to be safe. The binary representation must be 64-bots wide, a
	-- the @Int@ type to be safe. The binary representation must be 64-bits wide, a

	-- signed integral types. See thethis module's @convert@ function for more info.
	-- signed integral types. See this module's @convert@ function for more info.

		-- \|
		-- This call to @unsafeCoerce@ relies on the machine's binary representation of

	-- \|
	-- This call to @unsafeCoerce@ relies on the machine's binary representation of
	-- \| This call to @unsafeCoerce@ relies on the machine's binary representation of

	-- the same reason, the use must guarantee a 2's compliment representation for
	-- the same reason, the use must guarantee a 2's complement representation for

	-- with @Word64@ (such as 2's compliment); i.e.:
	-- with @Word64@ (such as 2's complement); i.e.:

Resolving TODO: PrimVar for UniqCounter #508

Resolving TODO: PrimVar for UniqCounter #508

Conversation

recursion-ninja commented Dec 22, 2024

Description

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoutts commented Jan 7, 2025

jorisdral commented Jan 7, 2025

dcoutts commented Jan 7, 2025

dcoutts left a comment

Choose a reason for hiding this comment

jorisdral commented Jan 7, 2025

dcoutts commented Jan 7, 2025