Add `UniqueSet`, remove direct name comparisons #5815

ana-pantilie · 2024-03-04T09:47:17Z

adds UniqueSet module
splits PlutusCore.Name into 3 modules: PlutusCore.Name.Unique (containing the main Name -> Unique logic), PlutusCore.Name.UniqueMap and PlutusCore.Name.UniqueSet which depend on Unique
reasoning for ^: to split common namespace, similar to the standard Map and Set; UniqueMap and UniqueSet should be imported qualified
replaces the standard Map and Set containers for names with UniqueMap and UniqueSet such that all comparisons are now done using Unique instead of Ord name

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Changelog fragments have been written (if appropriate)
- Relevant tickets are mentioned in commit messages
- Formatting, PNG optimization, etc. are updated
PR
- (For external contributions) Corresponding issue exists and is linked in the description
- Targeting master unless this is a cherry-pick backport
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

Signed-off-by: Ana Pantilie <[email protected]>

…-set

Signed-off-by: Ana Pantilie <[email protected]>

michaelpj

Looks good, a few doc thoughts, and some musings about making the interfaces more generic by making more use of typeclasses/lenses.

michaelpj · 2024-03-06T10:24:08Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+import Data.IntSet.Lens qualified as IS
+import PlutusCore.Name.Unique (HasUnique (..), Unique (Unique))
+
+-- | A set containing uniques.


Why is this good? I have honestly forgotten, and it's not in the PR description, or more importantly, in the doc for this type. How will we remember when to use it or not? What were the considerations that led to us defining this? I'm sure you know - write it down!

Same applies to UniqueMap

michaelpj · 2024-03-06T10:25:03Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+  }
+  deriving newtype (Show, Eq, Semigroup, Monoid)
+
+-- | Insert a value by a unique.


These comments read as more "map-y" (indeed, I think they're just copied from the map version). I think it could just be "insert a unique". And similarly for the other comments.

michaelpj · 2024-03-06T10:25:53Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+
+-- | Insert a value by the unique of a name.
+insertByName :: (HasUnique name unique) => name -> UniqueSet unique -> UniqueSet unique
+insertByName = insertByUnique . view unique


tbh, I slightly question whether these functions pass the Fairbarn threshold?

I realise most of them existed already, I'm just questioning whether they should!

Other functions are defined based on insertByUnique and insertByName to avoid redoing all back and forth between the ints and the uniques and the names

michaelpj · 2024-03-06T10:25:58Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+  unique ->
+  UniqueSet unique ->
+  UniqueSet unique
+insertByUnique = coerce . IS.insert . coerce


last coerce is unnecessary?

It's necessary because it coerces the unique into the Key type used by IntSet.

Sorry, that was very amibguous! I meant the one that is applied last i.e. the one that is applied first 😅 I think we get back a UniqueSet unique from insert, so no need to coerce it again.

Oh, that's the IntSet insert, its result needs to be wrapped into a UniqueSet.

michaelpj · 2024-03-06T10:31:48Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+memberByName = memberByUnique . view unique
+
+notMemberByName :: (HasUnique name unique) => name -> UniqueSet unique -> Bool
+notMemberByName n = not . memberByName n


surely this is not needed

The standard Set API has it https://hackage.haskell.org/package/containers-0.7/docs/Data-Set.html#v:notMember.

I guess the reason for defining this is that you might want to use it as an operator: x notMember s. I think not $ member x s is a tiny bit harder to read. 😄 That's why I chose to add notMemberByName, but I can remove it if you prefer.

michaelpj · 2024-03-06T10:34:12Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueMap.hs

+restrictKeys (UniqueMap m) (UniqueSet s) =
+  UniqueMap $ IM.restrictKeys m s
+
+foldr :: (a -> b -> b) -> b -> UniqueMap unique a -> b


We could implement https://hackage.haskell.org/package/mono-traversable-1.0.17.0/docs/Data-MonoTraversable.html#t:MonoFoldable for these types?

I couldn't get this to work quickly, not sure what's wrong. I'll revisit it the next time I need to do something with UniqueMap/Set (which is very soon).

I'm confused, how do we not get that with simple deriving Foldable?

michaelpj · 2024-03-06T10:35:13Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueMap.hs

+singletonByName :: (HasUnique name unique) => name -> a -> UniqueMap unique a
+singletonByName n a = insertByName n a mempty
+
+-- | Insert a named value by the index of the unique of the name.


I'm slightly unsure of your use of "index" here? I think you just mean by the unique? Applies throughout

Oops, I forgot to fix the docs! Thanks for pointing it out.

michaelpj · 2024-03-06T10:36:17Z

plutus-core/plutus-core/src/PlutusCore/Name/Unique.hs


  -- * Functions
-  insertByUnique,


michaelpj · 2024-03-06T10:37:38Z

plutus-core/plutus-core/src/PlutusCore/Subst.hs

@@ -221,35 +223,45 @@ programMapNames f g (Program a v term) = Program a v (termMapNames f g term)
 -- Free variables

 -- | Get all the free term variables in a term.
-fvTerm :: Ord name => Traversal' (Term tyname name uni fun ann) name
+fvTerm :: HasUnique name unique => Traversal' (Term tyname name uni fun ann) name


This is actually a better signature full stop. Ord name suggests it might work for e.g. de bruijn names, but it really won't.

michaelpj · 2024-03-06T10:40:52Z

plutus-core/untyped-plutus-core/src/UntypedPlutusCore/Transform/Inline.hs


 lookupVarInfo ::
  (HasUnique name TermUnique) =>
  name ->
  S name uni fun a ->
  Maybe (VarInfo name uni fun a)
-lookupVarInfo n s = lookupName n $ s ^. vars
+lookupVarInfo n s = UMap.lookupName n $ s ^. vars


Maybe we should implement https://hackage.haskell.org/package/lens-5.2.3/docs/Control-Lens-Combinators.html#t:At for UniqueMap?

Same as #5815 (comment)

Signed-off-by: Ana Pantilie <[email protected]>

effectfully

Sorry for such a late review, I'll try to do better next time.

All looks great to me, comments are nitpicks and thinking out loud.

effectfully · 2024-03-06T21:04:23Z

plutus-core/plutus-core/src/PlutusCore/Name/Unique.hs

+{- | Defines the 'Name' type used for identifiers in Plutus Core together with a technique
+ to minimise the cost of 'Name' comparisons.
+
+ A 'Name' is a piece of text used to identify a variable inside the Plutus Core languages.


Technically, it's the Unique that identified a Name, Text is there just for human readability.

effectfully · 2024-03-06T21:06:01Z

plutus-core/plutus-core/src/PlutusCore/Name/Unique.hs

+ therefore, compare the integers instead, which is obviously much more cost-effective.
+
+ We distinguish between the names of term variables and type variables by defining wrappers
+ over 'Name': 'TermName' and 'TyName'. Since the code we usually write is polymorphic in the


There's no TermName type as far as I'm aware. TermName appears as a constructor in a few places, but that's irrelevant.

effectfully · 2024-03-06T21:06:51Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueMap.hs

+  UniqueMap (..),
+insertByUnique,
+insertByName,
+singletonByName,


Nit: that's some weird formatting.

effectfully · 2024-03-06T21:09:14Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueMap.hs

+restrictKeys (UniqueMap m) (UniqueSet s) =
+  UniqueMap $ IM.restrictKeys m s
+
+foldr :: (a -> b -> b) -> b -> UniqueMap unique a -> b


I'm confused, how do we not get that with simple deriving Foldable?

effectfully · 2024-03-06T21:10:02Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueMap.hs

+
+{-# INLINE isEmpty #-}
+isEmpty :: UniqueMap unique a -> Bool
+isEmpty (UniqueMap m) = IM.null m


... and that as well, surely it's just null assuming we derive a Foldable instance for UniqueMap unique.

effectfully · 2024-03-06T21:15:38Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueMap.hs

+newtype UniqueMap unique a = UniqueMap
+  { unUniqueMap :: IM.IntMap a
+  }
+  deriving newtype (Show, Eq, Semigroup, Monoid, Functor)


I probably originally added those Semigroup and Monoid instances just to be able to use mempty. Semigroup is kinda weird for IntMap: instead of doing <> pointwise it simply drops elements from the second map when their keys exist in the first one, which can backfire. Would be good to check if we can remove those instances, although I guess since it was me who introduced that tech debt, I should probably do that myself.

Tried to remove the instances, doesn't seem trivial unfortunately. I'll open an issue.

effectfully · 2024-03-06T21:17:21Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+newtype UniqueSet unique = UniqueSet
+  { unUniqueSet :: IS.IntSet
+  }
+  deriving newtype (Show, Eq, Semigroup, Monoid)


On the other hand these instances here are completely fine and we probably should have consistent behavior between UniqueMap and UniqueSet. I wonder if this is why containers developers chose a somewhat weird instance for Semigroup (UniqueMap a).

effectfully · 2024-03-06T21:26:13Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+singletonName n = insertByName n mempty
+
+-- | Convert a 'Foldable' into a 'UniqueSet' using the given insertion function.
+fromFoldable ::


I would just do something like foldMap (singleton . coerce) or something

foldMap is usually lazy, so that one can derive foldr in terms of it for a right-nested structure and just generally define lazy functions, as well as strict ones. foldl' in terms of foldr is easy, foldr in terms of foldl' is literally one of my challenges, so you know it's evil.

foldl' here is gonna be more efficient for a right-nested structure than foldMap (singleton . coerce) and isn't any more hard to define. In the ideal world we'd generalize this to left-nested structures (if that's possible, I don't remember ever doing that) and use tree-like folding, but who cares, foldl' should be enough for most practical use cases and we can always improve it if it ever becomes a problem.

effectfully · 2024-03-06T21:35:27Z

plutus-core/plutus-core/src/PlutusCore/Name/UniqueSet.hs

+  Getting (UniqueSet unique) s name ->
+  s ->
+  UniqueSet unique
+setOfByName l = views l singletonName


I looked at Data.Set.Lens, got confused and gonna state that I trust that you got this one right :)

effectfully · 2024-03-06T21:45:39Z

Oh and thanks a lot for documenting existing code, that's extremely helpful.

Signed-off-by: Ana Pantilie <[email protected]>

ana-pantilie added 5 commits March 4, 2024 11:46

Add UniqueSet; use in UPLC.FloatDelay

945d8fb

Signed-off-by: Ana Pantilie <[email protected]>

Merge remote-tracking branch 'origin/master' into ana/plt-8647-unique…

cc94b3c

…-set

Use UniqueMap and UniqueSet in PIR.LetFloatOut

d5f0af6

Signed-off-by: Ana Pantilie <[email protected]>

Split PlutusCore.Name into multiple modules

b42ef7b

Signed-off-by: Ana Pantilie <[email protected]>

Clean-up

e1bc9fe

Signed-off-by: Ana Pantilie <[email protected]>

ana-pantilie marked this pull request as ready for review March 5, 2024 11:41

ana-pantilie changed the title ~~Add UniqueSet~~ Add UniqueSet, remove Ord name constraints Mar 5, 2024

ana-pantilie changed the title ~~Add UniqueSet, remove Ord name constraints~~ Add UniqueSet, remove direct name comparisons Mar 5, 2024

ana-pantilie requested review from effectfully and a team March 5, 2024 11:54

michaelpj approved these changes Mar 6, 2024

View reviewed changes

Improve documentation

23ae164

Signed-off-by: Ana Pantilie <[email protected]>

ana-pantilie added the No Changelog Required Add this to skip the Changelog Check label Mar 6, 2024

ana-pantilie merged commit b4f045f into master Mar 6, 2024
5 of 6 checks passed

ana-pantilie deleted the ana/plt-8647-unique-set branch March 6, 2024 17:33

effectfully reviewed Mar 6, 2024

View reviewed changes

anitajov assigned ana-pantilie Mar 6, 2024

ana-pantilie mentioned this pull request May 24, 2024

Remove or reimplement Semigroup, Monoid for UniqueMap #6138

Open

v0d1ch pushed a commit to v0d1ch/plutus that referenced this pull request Dec 6, 2024

Add UniqueSet, remove direct name comparisons (IntersectMBO#5815)

2f1a679

Signed-off-by: Ana Pantilie <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `UniqueSet`, remove direct name comparisons #5815

Add `UniqueSet`, remove direct name comparisons #5815

ana-pantilie commented Mar 4, 2024 •

edited

Loading

michaelpj left a comment

michaelpj Mar 6, 2024

michaelpj Mar 6, 2024

michaelpj Mar 6, 2024

michaelpj Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

effectfully Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

michaelpj Mar 6, 2024

michaelpj Mar 6, 2024

michaelpj Mar 6, 2024

ana-pantilie Mar 6, 2024

effectfully left a comment

effectfully Mar 6, 2024

effectfully Mar 6, 2024

effectfully Mar 6, 2024

effectfully Mar 6, 2024

effectfully Mar 6, 2024

effectfully Mar 6, 2024

ana-pantilie Mar 22, 2024

effectfully Mar 6, 2024

effectfully Mar 6, 2024

effectfully Mar 6, 2024

effectfully commented Mar 6, 2024

Add UniqueSet, remove direct name comparisons #5815

Add UniqueSet, remove direct name comparisons #5815

Conversation

ana-pantilie commented Mar 4, 2024 • edited Loading

michaelpj 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effectfully 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effectfully commented Mar 6, 2024

Add `UniqueSet`, remove direct name comparisons #5815

Add `UniqueSet`, remove direct name comparisons #5815

ana-pantilie commented Mar 4, 2024 •

edited

Loading