-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hashable-independent Aeson instances #1047
Conversation
A somewhat open question is the amount of inlining for the aeson instances. This affects compilation times. I think, the aeson package itself removed all inlining in recent version, to reduce compilation times. On the other hand inlining can make a big difference in runtime performance. The current version of the PR does not inline some larger instances in particular in cyclic data type dependencies. |
instance ToJSON a => ToJSON (SigData a) where | ||
toJSON (SigData h s c) = object $ concat | ||
[ ["hash" .= h] | ||
, ["sigs" .= object (map (\(k,ms) -> (unPublicKeyHex k, maybe Null (toJSON . _usSig) ms)) s)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instance seems to be in conflict with the comment above:
, _sigDataSigs :: [(PublicKeyHex, Maybe UserSig)]
-- ^ This is not a map because the order must be the same as the signers inside the command.
Should that be fixed? Would it break backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Pact/Types/SigData.hs
Outdated
, "sigs" .= HM.fromList (bimap unPublicKeyHex (fmap _usSig) <$> _sigDataSigs o) | ||
-- FIXME fix order independently of hashable package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use LegacyHashMap
for backward compatible behavior. Use list for pairs for behavior that complies with the comment on the _sigDataSigs
constructor field.
@jmcardon what would be the correct behavior here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this falls under ApiReq
(maybe grep to be sure) which means it doesn't show up on blockchain, thus low-stakes what we do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Main themes in review:
-
promote
LegacyValue
toJsonValue
(and move to aTypes
file), see discussion on type. -
Can we consider a
Generic
solution that somehow leverages aJsonProperties
value? Would cut code down. -
We have enough "elide on
Foldable.null
" cases to merit support a laMaybe
support, let's consider generalizing. -
Are we removing
enableToJSON
in this PR? -
Consider rename also for
LegacyHashMap
although here it really is legacy ... at a minimumLegacyHashed
is a good name.
@@ -46,6 +46,7 @@ import Data.ByteString (ByteString) | |||
import qualified Data.ByteString.Lazy.Char8 as BSL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is good work, we should consider not migrating this just to make the PR smaller -- ApiReq
never results in code on the blockchain, and while it's perhaps nice for Chainweaver and tools, we can't guarantee a particular coding order in JS clients etc.
src/Pact/Utils/Json.hs
Outdated
-- -------------------------------------------------------------------------- -- | ||
-- | ||
|
||
newtype LegacyValue = LegacyValue { getLegacyValue :: Value } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this. Legacy makes it seem like an artifact of the past that will soon be deprecated.
This has been merged via #1242 |
Overview
Backward & forward compatibility
The dependencies with the greatest consequences for preserving legacy behavior are JSON encoding and text processing.
JSON
Modern
aeson
(>= 2.0
) uses a different algorithm for sorting the fields of objects than our older pinned version ofaeson
. A large part of this PR is makingpact
use thepact-json
library for all serialization and deserialization.pact-json
focuses on explicit, predictable encoding over programmer convenience, and it provides helpers for encoding into the legacy format.Source parsing
Modern versions of
text
,trifecta
andattoparsec
bring various API changes and changes to internal logic. In particular the handling of unicode character counting changed whentext
moved from utf-16 to utf-8 for its internal representation. This PR introduces an abstracted parser that shims the new behavior to produce character counts compatible with the older versions.Performance regressions
Recent runs of the pact benchmarks are available for this branch (ghc-9.6.1) and master (ghc-8.10.7). Most benchmarks are slightly faster in the newer version. The largest benchmark, which compiles and runs a small currency-exchange contract, goes from 3.7 to 1.7 seconds uncached, 0.22 to 0.18 seconds cached. Performance is worse for the new branch when using an in-memory database. These results need to be replicated on other machines.
TODO: check the time required to run replay on this branch, compared to
master
.ghc-9.6.1:
![ghc961](https://private-user-images.githubusercontent.com/993484/239394293-0079b535-2109-4168-8df4-c2ff4bcf9e16.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjI0NTAzODksIm5iZiI6MTcyMjQ1MDA4OSwicGF0aCI6Ii85OTM0ODQvMjM5Mzk0MjkzLTAwNzliNTM1LTIxMDktNDE2OC04ZGY0LWMyZmY0YmNmOWUxNi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwNzMxJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDczMVQxODIxMjlaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1hZjRlNTM4OGFlNWZlN2E2NTFmYmE2NjBmZmU2M2Q5Y2Y0MzlmZTNiY2NjMTcxOGVhYjljOWMxOGIzNWFiZGI1JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.3km2rTC_7Yv-DgBvIzkAC3rK6xjch0QCWecbWAHCS98)
ghc-8.10.7:
![ghc8107](https://private-user-images.githubusercontent.com/993484/239394367-629147e6-67c8-4b9f-a2e4-5862c001f30e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjI0NTAzODksIm5iZiI6MTcyMjQ1MDA4OSwicGF0aCI6Ii85OTM0ODQvMjM5Mzk0MzY3LTYyOTE0N2U2LTY3YzgtNGI5Zi1hMmU0LTU4NjJjMDAxZjMwZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwNzMxJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDczMVQxODIxMjlaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0wNDJjNjIzOGNkOTFjMjQ3Y2EyMzlkYzk3MWRmYzQ0YjlhMzI1NDU5NGE3ZWNlMjE5M2JkNThkM2QyZTQ1NjNiJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.3xYa608kRiMGOKX1_Xn6wYULsTDbPST_E-UPW0PmjJM)