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

core/rawdb, ethdb: introduce batched/atomic reads from ancients #23566

Merged
merged 13 commits into from
Oct 25, 2021

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Sep 13, 2021

In several places, we use the following mechanic:

  1. Check if something exists in freezer,
  2. If not, check if it exists in leveldb,
  3. If not, check again if it exists in freezer,

This is because between 1 and 2, there's a (ever so slight) chance that it was written into ancients and deleted from leveldb.

As of #23462, we now do those types of transfers using ancient batches, and thus introduced the ability to do locked writes. This PR extends that ability to support locked reads.

This could be used in the following situations, and both simplify and reduce the code:

  • ReadCanonicalHash
  • ReadHeaderRLP
  • ReadBodyRLP
  • ReadCanonicalBodyRLP
  • ReadTdRLP
  • ReadReceiptsRLP

@holiman
Copy link
Contributor Author

holiman commented Sep 13, 2021

From a design-perspective, maybe the interfaces should look differently. This PR so far just adds AtomicReadAncients(fn func(AncientReader) error) (err error) to the AncientReader interface -- which adds an interface recursion in theory, but which doesn't hold in practice.

So theoretically, this PR allows to do this:

	db.AtomicReadAncients(func(reader ethdb.AncientReader) error {
		reader.AtomicReadAncients(func(reader ethdb.AncientReader) error {
			reader.AncientSize("foo")
		})
	})

In practice, that would lead to a double-RLock(). So maybe there should be an AncientReader with the base operations, and an AncientAtomicReader which is AncientReader + AtomicReadAncients ?

Or maybe it should be something else entirely.

@holiman
Copy link
Contributor Author

holiman commented Sep 16, 2021

In practice, that would lead to a double-RLock(). So maybe there should be an AncientReader with the base operations, and an AncientAtomicReader which is AncientReader + AtomicReadAncients ?

This is now fixed.

ethdb/database.go Outdated Show resolved Hide resolved
var data []byte
db.AtomicReadAncients(func(reader ethdb.AncientReader) error {
// Check if the data is in ancients
if h, err := reader.Ancient(freezerHashTable, number); err == nil && common.BytesToHash(h) == hash {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use bytes.Equal instead of BytesToHash

core/rawdb/accessors_chain.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Looks really nice so far!

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

Have we ever run the fast/snaps sync with this PR? My concern is that previously ancient writes and reads can happen concurrently but now it's not allowed. Ancient read is pretty common during the sync. Not sure how much performance will be affected.

@@ -90,6 +90,14 @@ type AncientReader interface {
AncientSize(kind string) (uint64, error)
}

// AncientBatchReader is the interface for 'batched' or 'atomic' reading.
type AncientBatchReader interface {
AncientReader
Copy link
Member

Choose a reason for hiding this comment

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

Please add one more line here.

Copy link
Member

Choose a reason for hiding this comment

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

AncientBatchReader looks confusing. Maybe we can rename the AncientReader to RawAncientReader and rename the AncientBatchReader to AncientReader.

core/rawdb/accessors_chain.go Outdated Show resolved Hide resolved
@@ -90,6 +90,14 @@ type AncientReader interface {
AncientSize(kind string) (uint64, error)
}

// AncientBatchReader is the interface for 'batched' or 'atomic' reading.
type AncientBatchReader interface {
AncientReader
Copy link
Member

Choose a reason for hiding this comment

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

AncientBatchReader looks confusing. Maybe we can rename the AncientReader to RawAncientReader and rename the AncientBatchReader to AncientReader.

AncientReader
// ReadAncients runs the given read operation while ensuring that no writes take place
// on the underlying freezer.
ReadAncients(fn func(AncientReader) error) (err error)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps ReadAncientsLocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ReadAncients operation is analoguous to ModifyAncients which is the locked/atomic/batched write operation.

@holiman
Copy link
Contributor Author

holiman commented Sep 28, 2021

Have we ever run the fast/snaps sync with this PR? My concern is that previously ancient writes and reads can happen concurrently but now it's not allowed

Ancient writes are already non-concurrent. But yes, you might have a point regarding the reads.

I've done snap syncs with PR's on top of this one, but I haven't run a head-to-head benchmark. I'll spin one up

@holiman
Copy link
Contributor Author

holiman commented Sep 28, 2021

This PR running on bench03 versus master on bench04, https://geth-bench.ethdevops.io/d/Jpk-Be5Wk/dual-geth?orgId=1&from=1632830932283&to=now

@holiman
Copy link
Contributor Author

holiman commented Sep 28, 2021

looks fine (pr is yellow)
Screenshot 2021-09-28 at 15-31-09 Dual Geth - Grafana

@holiman
Copy link
Contributor Author

holiman commented Sep 29, 2021

The syncs were pretty much same-same
Screenshot 2021-09-29 at 19-02-44 Dual Geth - Grafana

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

We had a review call about this, and here are my thoughts: This PR is good because it removes the complexity of having to think about concurrent modifications of the freezer while reading it.

The atomic read possibility (ReadAncients) is added here without removing the previous non-atomic freezer API. The practical difference between these two ways of accessing the freezer is subtle and related to freezer internals. I think that's not great because it increases the complexity of the API. But keeping non-atomic access as a possibility is the correct choice: we should still be able to write ancient chain data while other peers read headers.

@holiman holiman added this to the 1.10.12 milestone Oct 25, 2021
@holiman holiman merged commit 0e7efd6 into ethereum:master Oct 25, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Oct 25, 2021
…reum#23566)

This PR adds a new accessor method to the freezer database. This new view offers a consistent interface, guaranteeing that all individual tables (headers, bodies etc) are all on the same number, and that this number is not changes (added/truncated) while the operation is performing.
@holiman holiman deleted the atomic_ancients branch November 10, 2021 18:32
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
…reum#23566)

This PR adds a new accessor method to the freezer database. This new view offers a consistent interface, guaranteeing that all individual tables (headers, bodies etc) are all on the same number, and that this number is not changes (added/truncated) while the operation is performing.
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