Fix inlining for CompactAddr in the FromCBOR instance #2761
Conversation
| @@ -30,8 +36,9 @@ import Test.QuickCheck.Gen (chooseWord64) | |||
|
|
|||
| propValidateNewDecompact :: forall crypto. CC.Crypto crypto => Addr crypto -> Property | |||
| propValidateNewDecompact addr = | |||
| let compact = SBS.toShort $ serialiseAddr addr | |||
| decompactedOld = CA.deserializeShortAddr @crypto compact | |||
There was a problem hiding this comment.
Can you explain the change here? This seems like we're comparing to a different method than before. I am generally worried that we have so many different deserialisers now, it's very difficult to know which to use.
There was a problem hiding this comment.
Very good question and a very good observation: there are indeed too many deserializers now. In fact all this recent work paves the way for us remove all the old deserializers, but only after we are past the hardfork.
With regards to what has changed here is deserialiseAddr and deserializeShortAddr are almost identical, except latter works on ShortByteStrings, while the former works on ByteStrings. So, considering that deserializeShortAddr was used only for this test, we can just get rid of that function now and replace it with semantically equivalent one
| -- Nothing -> cborError $ DecoderErrorCustom "Addr" "invalid address" | ||
| fromCBOR = do | ||
| (_addr, cAddr) <- fromCborBackwardsBothAddr | ||
| pure cAddr |
There was a problem hiding this comment.
This was mainly an experiment to check if for some reason fmap snd is too lazy for the Decoder and forcing the tuple would make an improvement. Honestly I don't know if it did, because I was also adding INLINE pragmas as well. As soon as I saw a regression gone, I didn't bother going back and changing it back to snd <$> fromCborBackwardsBothAddr, since that would mean at least another hour being wasted. However, if you are really curious I can make an effort and do a db-analyser run with this change reverted.
|
@nc6 Hopefully I've answered your questions. I've restarted the hydra build, since there was one of those recurring false positive test failures, however I have no idea why hydra is reporting CI as failing. Maybe you know you know how to fix it, if not, I'll force push a commit tomorrow. |
There was not enough inlining prior to this PR which resulted in a slight but noticeable regression, when comparing to the implementation prior to the fix to address deserialization. TLDR, nothing crazy, just some inlining.