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

perf: cache system cell for resolve deps #2006

Merged
merged 2 commits into from
Apr 21, 2020
Merged

perf: cache system cell for resolve deps #2006

merged 2 commits into from
Apr 21, 2020

Conversation

zhangsoledad
Copy link
Member

@zhangsoledad zhangsoledad commented Apr 13, 2020

The goal of this PR is to optimize transaction resolve.
Currently, transaction resolve is inefficient, cause system-cell is uncached, every transaction depends on system cell will read system-cell from disk repeatedly. this PR proposal a system-cell cache implementation.

@zhangsoledad zhangsoledad requested review from a team and xxuejie April 13, 2020 12:06
@zhangsoledad
Copy link
Member Author

benchmark

@yangby-cryptape
Copy link
Collaborator

Benchmark Result

  • TPS: 377.90
  • Samples Count: 50
  • CKB Version: e72ebc7
  • Instance Type: c5.xlarge
  • Instances Count: 3
  • Bench Type: 2in2out
  • CKB Logger Filter: info,ckb=debug

@doitian
Copy link
Member

doitian commented Apr 14, 2020

Please add PR description:

  • Why this PR is needed
  • What this PR has done
  • The comparison

@@ -424,16 +446,27 @@ pub fn resolve_transaction<CP: CellProvider, HC: HeaderChecker, S: BuildHasher>(
}
}

let system_cell = SYSTEM_CELL.get().expect("SYSTEM_CELL is not initialized");
Copy link
Member

Choose a reason for hiding this comment

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

I think a more elegant solution is creating a wrapper around the resolver, like:

struct<F> CellProviderWithSystemCellCache {
  system_cell: SystemCell,
  inner: F,
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is a draft for bench, I will improve it later.
But I think CellProvider is also a bad idea, this cache is for dep, not input.

@zhangsoledad zhangsoledad changed the title perf: cache system cell for resolve deps [HOLD] perf: cache system cell for resolve deps Apr 14, 2020
nervos-bot[bot]
nervos-bot bot previously requested changes Apr 14, 2020
Copy link

@nervos-bot nervos-bot bot left a comment

Choose a reason for hiding this comment

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

Hold as requested by @zhangsoledad.

@zhangsoledad
Copy link
Member Author

benchmark

@yangby-cryptape
Copy link
Collaborator

Benchmark Result

  • TPS: 381.56
  • Samples Count: 51
  • CKB Version: 0c86b9f
  • Instance Type: c5.xlarge
  • Instances Count: 3
  • Bench Type: 2in2out
  • CKB Logger Filter: info,ckb=debug

@zhangsoledad zhangsoledad changed the title [HOLD] perf: cache system cell for resolve deps perf: cache system cell for resolve deps Apr 20, 2020
@nervos-bot nervos-bot bot dismissed their stale review April 20, 2020 02:48

Unhold as requested by @zhangsoledad.

@quake
Copy link
Member

quake commented Apr 21, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 21, 2020

Build succeeded:

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 07edc0b into develop Apr 21, 2020
@bors bors bot deleted the op-resolve-dep branch April 21, 2020 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants