Skip to content
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

Statically-known Text representations for `ChainwebVersion` #153

Merged
merged 7 commits into from Apr 18, 2019

Conversation

Projects
None yet
3 participants
@fosskers
Copy link
Contributor

commented Apr 16, 2019

This PR allows the usage of ChainwebVersion text representations like testWithTime-peterson instead of testWithTime-4075880449. This applies to CLI flags, as well as JSON.

In turn, this fixes the "version Map priming" problem, which blocks further work elsewhere (#135).

The new accepted postfixes are:

  • -singleton
  • -pair
  • -triangle
  • -peterson
  • -twenty
  • -hoffman-singleton

fosskers added some commits Apr 16, 2019

, ("-peterson", petersonChainGraph)
, ("-twenty", twentyChainGraph)
, ("-hoffman-singleton", hoffmanSingletonGraph)
]

This comment has been minimized.

Copy link
@fosskers

fosskers Apr 16, 2019

Author Contributor

Anything special I need to do to make sure these CAFs stay in unGC'd?

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy Apr 16, 2019

Contributor

CAFs get GCd???

This comment has been minimized.

Copy link
@fosskers

fosskers Apr 17, 2019

Author Contributor

This is my paranoia, I personally have no answer to that. Do we know that CAFs stay around forever, and are only evaluated once?

, ("-peterson", petersonChainGraph)
, ("-twenty", twentyChainGraph)
, ("-hoffman-singleton", hoffmanSingletonGraph)
]

This comment has been minimized.

Copy link
@larskuhtz

larskuhtz Apr 16, 2019

Contributor

It seems that this map is consulted each time a chainweb version is serialized or deserialized.

I am a bit concerned about mixing test and production versions here. In the previous version of the code only test versions uses special code paths. The lookup for production version was very straight forward and efficient.

What about keeping the old implementation of chainwebVersionFromText and just adapt the case

-- Generic test versions
	--
	chainwebVersionFromText t = ... 

for test versions?

This comment has been minimized.

Copy link
@fosskers

fosskers Apr 17, 2019

Author Contributor

It seems that this map is consulted each time a chainweb version is serialized or deserialized.

That old code also consulted the TVar'd Map for each serialization, too (at least for test versions).

This comment has been minimized.

Copy link
@larskuhtz

larskuhtz Apr 17, 2019

Contributor

Yes, that's my point: It did so only for test versions.


instance HasTextRepresentation ChainwebVersion where
toText = chainwebVersionToText
{-# INLINE toText #-}
fromText = chainwebVersionFromText
{-# INLINE fromText #-}

chainwebVersions :: HM.HashMap T.Text ChainwebVersion
chainwebVersions = HM.fromList $

This comment has been minimized.

Copy link
@larskuhtz

larskuhtz Apr 16, 2019

Contributor

This builds a map for every possible combination of version stem and graph. Would it be possible to reduce the map to just the choices of graphs and build the combined test chainweb version on the fly?

This comment has been minimized.

Copy link
@larskuhtz

larskuhtz Apr 16, 2019

Contributor

hmm, maybe it doesn't matter that much... it's only a factor of three...

Show resolved Hide resolved src/Chainweb/Version.hs Outdated
Show resolved Hide resolved src/Chainweb/Version.hs Outdated
@larskuhtz
Copy link
Contributor

left a comment

👍 for removing Simulation and removing unqualified use of Data.DiGraph.

For readability and performance I would prefer if we would leave the code outside of the -- Test Instances section mostly unchanged. The goal should be to replace the current mutable and dynamically created map with a static map of known graph versions/ version-ids. Also the use of hash for the tagging the version-id with the graph should be replaced by static id-tags for the known graphs.

The code outside of the --Test Instances section should be as straight forward and maintainable as possible and all complexity related to test versions should be quarantined in the respective section of the module.

@fosskers

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

I'll see what I can rework.

Use types to hardcode known ChainGraph information
This lets us avoid calling `hash` frequently during testing scenarios.
Testnet00 -> "testnet00"
chainwebVersionToText :: HasCallStack => ChainwebVersion -> T.Text
chainwebVersionToText Testnet00 = "testnet00"
chainwebVersionToText v = fromJuste $ HM.lookup v prettyVersions

This comment has been minimized.

Copy link
@fosskers

fosskers Apr 17, 2019

Author Contributor

Hm, this will be a source of silent pattern match failures. When someone creates a new production ChainwebVersion value, they won't be warned about the missing match here.

This comment has been minimized.

Copy link
@fosskers

fosskers Apr 17, 2019

Author Contributor

Hopefully quickcheck would catch it.

@fosskers fosskers merged commit 1977709 into master Apr 18, 2019

1 check passed

ci/gitlab/gitlab.com Pipeline passed on GitLab
Details

@fosskers fosskers deleted the colin/static-versions branch Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.