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

UTxO-HD: add tables to mock ledger #4184

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Nov 23, 2022

Description

Every test that used to run with SimpleBlocks now uses tables

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@jasagredo jasagredo force-pushed the jasagredo/add-tables-to-mock-ledger branch from 0da775b to bf42413 Compare November 23, 2022 14:58
Comment on lines 366 to 369
applyBlockLedgerResult _ blk st = fmap (\st' -> pureLedgerResult $ zipOverLedgerTables f (unstowLedgerTables st') (projectLedgerTablesTicked st)) . updateSimpleLedgerState blk $ TickedSimpleLedgerState $ stowLedgerTables $ getTickedSimpleLedgerState st
where f :: (Ord k, Eq v) => ValuesMK k v -> ValuesMK k v -> DiffMK k v
f val1 val2 = (\(ApplyTrackingMK _ d) -> ApplyDiffMK d) $ rawCalculateDifference val1 val2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already well known pattern of:

diff . unstow . apply . stow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I find that in every instance where we have to perform stowing/unstowing, applying, and diffing it's hard to track what's going on and why. I wonder if it'd make sense to abstract this away.

@jasagredo jasagredo self-assigned this Nov 23, 2022
@jasagredo jasagredo marked this pull request as ready for review November 23, 2022 14:59
@jasagredo jasagredo requested review from jorisdral and removed request for coot, nfrisby and bolt12 November 23, 2022 14:59
@jasagredo jasagredo added consensus issues related to ouroboros-consensus UTxO-HD 📒💽 labels Nov 23, 2022
Copy link
Contributor

@jorisdral jorisdral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Note: there are some lines that exceed ~80-ish characters, I placed some suggestions to spread them out over multiple lines, but I haven't done that everywhere

Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably asses the impact this has in testing time (just run the test before and after to see if there is a significant diff).

I think the changes of the mempool simplification are also here, right, so I skipped those. But let me know if I should review those.

Comment on lines 366 to 369
applyBlockLedgerResult _ blk st = fmap (\st' -> pureLedgerResult $ zipOverLedgerTables f (unstowLedgerTables st') (projectLedgerTablesTicked st)) . updateSimpleLedgerState blk $ TickedSimpleLedgerState $ stowLedgerTables $ getTickedSimpleLedgerState st
where f :: (Ord k, Eq v) => ValuesMK k v -> ValuesMK k v -> DiffMK k v
f val1 val2 = (\(ApplyTrackingMK _ d) -> ApplyDiffMK d) $ rawCalculateDifference val1 val2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I find that in every instance where we have to perform stowing/unstowing, applying, and diffing it's hard to track what's going on and why. I wonder if it'd make sense to abstract this away.

Copy link
Contributor Author

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check if there is an increase in the running time of the tests for introducing this.

Every test that used `SimpleBlock`s now runs with ledger tables.
@jasagredo jasagredo force-pushed the jasagredo/add-tables-to-mock-ledger branch from ac1e158 to 571e664 Compare December 1, 2022 11:08
@jasagredo
Copy link
Contributor Author

jasagredo commented Dec 1, 2022

test-consensus

feature/utxo-hd

All 59 tests passed (29.12s)
	User time (seconds): 25.58
	System time (seconds): 0.33
	Percent of CPU this job got: 87%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:29.48
	Maximum resident set size (kbytes): 280364

jasagredo/add-tables-to-mock-ledger

All 59 tests passed (28.44s)
	User time (seconds): 24.36
	System time (seconds): 0.36
	Percent of CPU this job got: 85%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:28.80
	Maximum resident set size (kbytes): 198016

Remarks:

  • notable memory decrease to 2/3.
  • roughly equivalent running time.

ouroboros-consensus-mock-test

feature/utxo-hd

All 37 tests passed (94.93s)
	User time (seconds): 94.37
	System time (seconds): 0.96
	Percent of CPU this job got: 100%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 1:35.28
	Maximum resident set size (kbytes): 221724

jasagredo/add-tables-to-mock-ledger

All 37 tests passed (112.14s)
	User time (seconds): 111.27
	System time (seconds): 1.30
	Percent of CPU this job got: 100%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 1:52.50
	Maximum resident set size (kbytes): 248692

Remarks:

  • Similar memory usage
  • Time increase. It is consistent (tried again) but it is like 10% only, I think it is acceptable. If we wanted we could suppress the output of the threadnet tests and this should make it even faster.

@jasagredo jasagredo merged commit 60f52b0 into feature/utxo-hd Dec 1, 2022
@iohk-bors iohk-bors bot deleted the jasagredo/add-tables-to-mock-ledger branch December 1, 2022 11:14
@jasagredo jasagredo mentioned this pull request Dec 2, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus UTxO-HD 📒💽
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leverage existing mempool tests to make sure the new ledger interface is exercised properly
3 participants