Renamed to ASTSize, changed to Maybe CoverageIndex #6081

bezirg · 2024-05-22T13:14:38Z

My initial intentions was to stop using ast size in Code.sizePlc because it may be confusing,
but use flat byte size instead.

Instead I renamed Size related functions and modules to ASTSize to make it more clear what this size is supposed to be.

I will do the real switch to flat/cbor size in another PR.

Also CoverageIndex had 2 problems:

It was not wrapped in Maybe (when the coverageindex is missing) . So there was ambiguity when the coverageindex was empty: it could either mean the coverage yielded empty results or the coverage was never run.
It was not serialised in case of SerialisedCode

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

bezirg · 2024-05-28T12:29:26Z

plutus-benchmark/nofib/src/PlutusBenchmark/NoFib/Knights/Sort.hs

I changed this because I was compiling with -O0 , and it complained that there was no unfolding for GHC.++ in quickSort. Turns out this module was using too much of ghc stdlib. Is it good that I changed it @kwxm?

Hmmm. The Haskell programs that these are based on are about 30 years old and are probably a bit old-fashioned to modern eyes: a lot of stuff that's commonly used these days just didn't exist when they were written. I just ported them with minimal changes and resisted the temptation to start "improving" them. My inclination would be to leave them as they are because changing the source makes it harder to see what changes are due to changes in our compiler: the differences in this file (which are quite small!) seem to have made the PIR for the whole program quite significantly different (current version, new version), which presumably accounts for the budget changes that show up later. Unless the programs stop working completely it's probably better not to change them.

Unisay · 2024-05-31T07:33:04Z

plutus-benchmark/nofib/src/PlutusBenchmark/NoFib/Knights/Sort.hs

@@ -99,9 +101,9 @@ randomIntegers s1 s2 =
        if 1 <= s2 && s2 <= 2147483398 then
            rands s1 s2
        else
-            error "randomIntegers: Bad second seed."
+            error () -- "randomIntegers: Bad second seed."


Is there a reason not to use traceError "randomIntegers: Bad second seed."?

Is there a reason not to use traceError "randomIntegers: Bad second seed."?

I think the reason for that is that traceError didn't exist when these were written (or more accurately, were stolen from here). I don't think that error will ever occur anyway.

Unisay · 2024-05-31T08:49:50Z

plutus-benchmark/nofib/test/9.6/knights10-4x4.budget.golden

+({cpu: 2556922462
+| mem: 8409416})


Given the changes above, do you have an idea why did the budget go up?

Unisay · 2024-05-31T08:57:23Z

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

@@ -0,0 +1,66 @@
+module PlutusCore.ASTSize


Since this module is new I suggest to use:

2 space indentation, as per the CODESTYLE.adoc

Explicit imports.

About the 2space I agree, but about the explicit imports I kind of disagree.

I used to be back in the days also a proponent of explicit imports, but thanks to mpj I learned that managing and constantly editing the import list is a burden, even with hls helping you out (when it works).

I learned that managing and constantly editing the import list is a burden

I agree that its a burden for a writer, but its a relief for a reader. Wildcard import shift the effort away from writers to readers. Since code is read more times that its written, I'd argue that we should optimise for reading.

Unisay · 2024-05-31T09:00:07Z

plutus-core/plutus-ir/src/PlutusIR/ASTSize.hs

@@ -0,0 +1,25 @@
+module PlutusIR.ASTSize


Same comment about 2-space indentation and explicit imports.

Unisay · 2024-05-31T09:17:24Z

plutus-tx/src/PlutusTx/Code.hs

+    SerializedCode
+        BS.ByteString -- ^ UPLC.Program flat-encoded
+        (Maybe BS.ByteString) -- ^ PlutusIR.Program flat-encoded
+        (Maybe BS.ByteString) -- ^ CoverageIndex flat-encoded


Thanks for the comments!

Unisay · 2024-05-31T09:19:52Z

plutus-tx/src/PlutusTx/Code.hs

 getCovIdx wrapper = case wrapper of
-  SerializedCode _ _ idx   -> idx
+  SerializedCode _ _ idx   -> unsafeFromEither . unflat <$> idx


The error emitted by unsafeFromEither is not telling what invariant was violated, its too generic.
I suggest to pattern match and throw a more explanatory error, e.g. "SerializedCode was expected to contain information about CodeCoverage because of ... but it doesn't"

Unisay · 2024-05-31T09:21:21Z

plutus-tx/src/PlutusTx/TH.hs

@@ -29,7 +29,7 @@ loadFromFile fp = TH.liftSplice $ do
    -- We don't have a 'Lift' instance for 'CompiledCode' (we could but it would be tedious),
    -- so we lift the bytestring and construct the value in the quote.
    bs <- liftIO $ BS.readFile fp
-    TH.examineSplice [|| SerializedCode bs Nothing mempty ||]
+    TH.examineSplice [|| SerializedCode bs Nothing Nothing ||]


I this point I am wondering if there are any reasons not to make SerializedCode a record with fields and use them to increase readability?

I this point I am wondering if there are any reasons not to make SerializedCode a record with fields and use them to increase readability?

That might indeed be helpful.

Unisay

I left some suggestions but they aren't critical.

kwxm · 2024-06-03T12:20:38Z

/benchmark nofib

github-actions · 2024-06-03T12:20:51Z

Click here to check the status of your benchmark.

kwxm · 2024-06-03T12:25:53Z

Click here to check the status of your benchmark.

Failed :(

kwxm

This looks basically OK, but I'm not too keen on modifying the nofib Knights example if we can avoid it.

kwxm · 2024-06-03T11:07:56Z

plutus-benchmark/nofib/src/PlutusBenchmark/NoFib/Knights/Sort.hs

Hmmm. The Haskell programs that these are based on are about 30 years old and are probably a bit old-fashioned to modern eyes: a lot of stuff that's commonly used these days just didn't exist when they were written. I just ported them with minimal changes and resisted the temptation to start "improving" them. My inclination would be to leave them as they are because changing the source makes it harder to see what changes are due to changes in our compiler: the differences in this file (which are quite small!) seem to have made the PIR for the whole program quite significantly different (current version, new version), which presumably accounts for the budget changes that show up later. Unless the programs stop working completely it's probably better not to change them.

kwxm · 2024-06-03T11:19:19Z

plutus-benchmark/nofib/src/PlutusBenchmark/NoFib/Knights/Sort.hs

@@ -99,9 +101,9 @@ randomIntegers s1 s2 =
        if 1 <= s2 && s2 <= 2147483398 then
            rands s1 s2
        else
-            error "randomIntegers: Bad second seed."
+            error () -- "randomIntegers: Bad second seed."


Is there a reason not to use traceError "randomIntegers: Bad second seed."?

I think the reason for that is that traceError didn't exist when these were written (or more accurately, were stolen from here). I don't think that error will ever occur anyway.

kwxm · 2024-06-03T11:26:46Z

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

+import Control.Lens
+import Data.Monoid
+
+newtype ASTSize = ASTSize


Just to be annoying, maybe ASTSize isn't as informative as it could be: there might be lots of different ways you could measure the size of an AST and the name doesn't make it obvious what's actually being measured. Something likeNumberOfASTNodes might be more informative. I don't mind all that much though.

kwxm · 2024-06-03T13:55:04Z

plutus-tx/src/PlutusTx/TH.hs

@@ -29,7 +29,7 @@ loadFromFile fp = TH.liftSplice $ do
    -- We don't have a 'Lift' instance for 'CompiledCode' (we could but it would be tedious),
    -- so we lift the bytestring and construct the value in the quote.
    bs <- liftIO $ BS.readFile fp
-    TH.examineSplice [|| SerializedCode bs Nothing mempty ||]
+    TH.examineSplice [|| SerializedCode bs Nothing Nothing ||]


I this point I am wondering if there are any reasons not to make SerializedCode a record with fields and use them to increase readability?

That might indeed be helpful.

bezirg force-pushed the bezirg/astsize branch from ab9bf0f to 4c22f08 Compare May 22, 2024 13:33

bezirg changed the title ~~wip~~ Renamed to ASTSize, changed to Maybe CoverageIndex May 22, 2024

bezirg marked this pull request as ready for review May 22, 2024 13:33

bezirg requested a review from zliu41 May 22, 2024 13:33

bezirg self-assigned this May 22, 2024

bezirg force-pushed the bezirg/astsize branch from 4c22f08 to 0114f6d Compare May 22, 2024 13:47

bezirg requested a review from Unisay May 28, 2024 09:47

bezirg force-pushed the bezirg/astsize branch from 0114f6d to aebbd0c Compare May 28, 2024 12:27

bezirg requested review from kwxm and removed request for zliu41 May 28, 2024 12:28

bezirg commented May 28, 2024

View reviewed changes

bezirg force-pushed the bezirg/astsize branch from aebbd0c to 96dad05 Compare May 29, 2024 10:53

Unisay reviewed May 31, 2024

View reviewed changes

kwxm approved these changes Jun 3, 2024

View reviewed changes

Renamed to ASTSize, changed to Maybe CoverageIndex

f0b54a5

bezirg force-pushed the bezirg/astsize branch from 96dad05 to f0b54a5 Compare June 8, 2024 14:19

zeme-wana force-pushed the master branch 2 times, most recently from a161078 to db5cabb Compare July 9, 2024 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed to ASTSize, changed to Maybe CoverageIndex #6081

Renamed to ASTSize, changed to Maybe CoverageIndex #6081

bezirg commented May 22, 2024 •

edited

Loading

bezirg May 28, 2024

kwxm Jun 3, 2024

Unisay May 31, 2024

bezirg Jun 3, 2024

kwxm Jun 3, 2024

Unisay May 31, 2024

Unisay May 31, 2024

bezirg Jun 3, 2024

Unisay Jun 3, 2024

Unisay May 31, 2024

Unisay May 31, 2024

Unisay May 31, 2024

Unisay May 31, 2024

kwxm Jun 3, 2024

Unisay left a comment

kwxm commented Jun 3, 2024

github-actions bot commented Jun 3, 2024

kwxm commented Jun 3, 2024

kwxm left a comment

kwxm Jun 3, 2024

kwxm Jun 3, 2024

kwxm Jun 3, 2024

kwxm Jun 3, 2024

Renamed to ASTSize, changed to Maybe CoverageIndex #6081

Are you sure you want to change the base?

Renamed to ASTSize, changed to Maybe CoverageIndex #6081

Conversation

bezirg commented May 22, 2024 • edited Loading

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

Unisay left a comment

Choose a reason for hiding this comment

kwxm commented Jun 3, 2024

github-actions bot commented Jun 3, 2024

kwxm commented Jun 3, 2024

kwxm 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

bezirg commented May 22, 2024 •

edited

Loading