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

mempool: Refactor mempool code to its own package. #737

Merged
merged 1 commit into from
Aug 19, 2016

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Aug 15, 2016

This does the minimum work necessary to refactor the mempool code into its own package. The idea is that separating this code into its own package will greatly improve its testability, allow independent
benchmarking and profiling, and open up some interesting opportunities for future development related to the memory pool.

There are likely some areas related to policy that could be further refactored, however it is better to do that in future commits in order to keep the changeset as small as possible during this refactor.

Overview of the major changes:

  • Create the new package
  • Move several files into the new package:
    • mempool.go -> mempool/mempool.go
    • mempoolerror.go -> mempool/error.go
    • policy.go -> mempool/policy.go
    • policy_test.go -> mempool/policy_test.go
  • Update mempool logging to use the new mempool package logger
  • Rename mempoolPolicy to Policy (so it's now mempool.Policy)
  • Rename mempoolConfig to Config (so it's now mempool.Config)
  • Rename mempoolTxDesc to TxDesc (so it's now mempool.TxDesc)
  • Rename txMemPool to TxPool (so it's now mempool.TxPool)
  • Move defaultBlockPrioritySize to the new package and export it
  • Export DefaultMinRelayTxFee from the mempool package
  • Export the CalcPriority function from the mempool package
  • Introduce a new RawMempoolVerbose function on the TxPool and update
    the RPC server to use it
  • Update all references to the mempool to use the package
  • Add a skeleton README.md

@davecgh davecgh force-pushed the davec_mempool_package branch 3 times, most recently from de7feaf to b3f8262 Compare August 15, 2016 23:42
@dajohi
Copy link
Member

dajohi commented Aug 16, 2016

ok

This does the minimum work necessary to refactor the mempool code into
its own package.  The idea is that separating this code into its own
package will greatly improve its testability, allow independent
benchmarking and profiling, and open up some interesting opportunities
for future development related to the memory pool.

There are likely some areas related to policy that could be further
refactored, however it is better to do that in future commits in order
to keep the changeset as small as possible during this refactor.

Overview of the major changes:

- Create the new package
- Move several files into the new package:
  - mempool.go -> mempool/mempool.go
  - mempoolerror.go -> mempool/error.go
  - policy.go -> mempool/policy.go
  - policy_test.go -> mempool/policy_test.go
- Update mempool logging to use the new mempool package logger
- Rename mempoolPolicy to Policy (so it's now mempool.Policy)
- Rename mempoolConfig to Config (so it's now mempool.Config)
- Rename mempoolTxDesc to TxDesc (so it's now mempool.TxDesc)
- Rename txMemPool to TxPool (so it's now mempool.TxPool)
- Move defaultBlockPrioritySize to the new package and export it
- Export DefaultMinRelayTxFee from the mempool package
- Export the CalcPriority function from the mempool package
- Introduce a new RawMempoolVerbose function on the TxPool and update
  the RPC server to use it
- Update all references to the mempool to use the package.
- Add a skeleton README.md
func newTxMemPool(cfg *mempoolConfig) *txMemPool {
memPool := &txMemPool{
func New(cfg *Config) *TxPool {
return &TxPool{
cfg: *cfg,
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: why does the New() function accept a *Config, but then immediately deference it?

It seems that either New() should be changed to accept a Config, or the TxPool's cfg attribute be changed to be a *Config rather than a Config.

I know this behavior has been in place since the mempool was slightly refactored to use the previous mempoolConfig, but it's been bugging me since then... 🤓

Copy link
Member Author

@davecgh davecgh Aug 16, 2016

Choose a reason for hiding this comment

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

It's to avoid two copies although it's small and only done once, so it probably really doesn't matter.

The intent is to make sure once a config is provided it can't be changed by the caller hence the deref to make a copy. Accepting it as a parameter means that parameter is a copy and then setting the field in the type is another copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind changing it if it really bugs folks.

Copy link
Member

Choose a reason for hiding this comment

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

I see, I hadn't considered possible mutation of the config after it had been passed to the mempool.

With that knowledge, it no longer bugs me 🙂

Copy link
Member

@jrick jrick Aug 16, 2016

Choose a reason for hiding this comment

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

The config is not safe for concurrent access though, it should be treated as immutable.

edit: nvm I only read half this conversation, this is exactly what davec is trying to avoid by making a local copy.

@Roasbeef
Copy link
Member

The changes read well. I like how the commit is rather minimal in its refactorings.

This is a great step towards future mempool extensions such as shared distributed TxPool or a fully on-disk TxPool! Extracting the mempool into a separate package will much such changes easier in the future as pointed out in the PR description.

@davecgh davecgh merged commit 7fac099 into btcsuite:master Aug 19, 2016
@davecgh davecgh deleted the davec_mempool_package branch August 19, 2016 16:08
@davecgh davecgh mentioned this pull request Aug 19, 2016
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.

4 participants