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

Replace splitl/splitr by splitSized in DiffSeq module #4273

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

jorisdral
Copy link
Contributor

Description

Closes #4272.

We replace uses of splitl and splitr in the DiffSeq module by uses of splitSized. Furthermore, we run the existing DiffSeq benchmarking suite twice, once for the "old" version that uses splitl and splitr (i.e., baseline), and the new version that uses splitSized.

Baseline (a40b863):

...
            DiffSeq
              Configuration: default
                Comparative performance analysis
                  DiffSeq:                                OK (8.41s)
                    2.738 s ± 226 ms, 0.26x
                  SeqUtxoDiff:                            OK (75.67s)
                    10.558 s ± 212 ms
...
              Configuration: no switches
                Comparative performance analysis
                  DiffSeq:                                OK (6.56s)
                    2.043 s ±  16 ms, 0.22x
                  SeqUtxoDiff:                            OK (27.44s)
                    9.083 s ± 158 ms
...
              Configuration: only small switches
                Comparative performance analysis
                  DiffSeq:                                OK (6.83s)
                    2.129 s ±  44 ms, 0.23x
                  SeqUtxoDiff:                            OK (28.34s)
                    9.296 s ±  65 ms
...
              Configuration: no switches, larger flushes (every 100 pushes)
                Comparative performance analysis
                  DiffSeq:                                OK (6.35s)
                    2.043 s ±  47 ms, 0.30x
                  SeqUtxoDiff:                            OK (21.03s)
                    6.870 s ± 152 ms
...

Version using splitSized (8b63e53):

...
            DiffSeq
              Configuration: default
                Comparative performance analysis
                  DiffSeq:                                OK (7.74s)
                    2.415 s ±  13 ms, 0.24x
                  SeqUtxoDiff:                            OK (31.05s)
                    10.261 s ±  86 ms
...
              Configuration: no switches
                Comparative performance analysis
                  DiffSeq:                                OK (6.58s)
                    2.047 s ± 4.3 ms, 0.22x
                  SeqUtxoDiff:                            OK (27.87s)
                    9.177 s ± 395 ms
...
              Configuration: only small switches
                Comparative performance analysis
                  DiffSeq:                                OK (6.84s)
                    2.128 s ±  11 ms, 0.23x
                  SeqUtxoDiff:                            OK (28.40s)
                    9.320 s ± 103 ms
...
              Configuration: no switches, larger flushes (every 100 pushes)
                Comparative performance analysis
                  DiffSeq:                                OK (6.61s)
                    2.043 s ±  68 ms, 0.30x
                  SeqUtxoDiff:                            OK (20.78s)
                    6.775 s ± 111 ms
...

We see that for the first scenario, the splitSized version shows a small performance improvement. This is probably due to the fact that the default configuration benchmarking scenario sometimes performs large switches, which use splitr, and large switches trigger the worst-case scenario for splitr in terms of time complexity. The other three scenarios report identical/very similar performance.

Note that these results are not definitive proof of relative performance, but they do give a strong indication that using splitSized does not result in a significant performance decrease. I therefore suggest we go ahead with replacing splitl and splitr with splitSized.

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • The documentation has been properly updated
    • 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

@jorisdral jorisdral self-assigned this Jan 11, 2023
@jorisdral jorisdral added consensus issues related to ouroboros-consensus benchmarking UTxO-HD 📒💽 labels Jan 11, 2023
@jorisdral jorisdral changed the title Improved splits for DiffSeq datatype Replace splitl/splitr by splitSized in DiffSeq module Jan 11, 2023
@jorisdral jorisdral force-pushed the jdral/4272-improved-diffseq-splits branch from 8b63e53 to 0c4e576 Compare January 11, 2023 15:07
@jorisdral jorisdral merged commit ea6ec39 into feature/utxo-hd Jan 12, 2023
@jorisdral jorisdral deleted the jdral/4272-improved-diffseq-splits branch January 12, 2023 11:24
jasagredo pushed a commit that referenced this pull request Jan 18, 2023
…fseq-splits

Replace `splitl`/`splitr` by `splitSized` in `DiffSeq` module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarking consensus issues related to ouroboros-consensus UTxO-HD 📒💽
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate possibilities for using improved splits for anti-diff finger trees in consensus
3 participants