Skip to content

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jun 26, 2020

This is a rebase+squash of #20236.

This PR contains a massive refactoring in the downloader + queue area. It's not quite ready to be merged yet, I'd like to see how the tests perform.

Todo: add some more unit-tests regarding the resultstore implementation, and the queue.

Throttling

Previously, we had a doneQueue which was a map where we kept track of all downloaded items (receipts, block bodies). This map was updated when deliveries came in, and cleaned when results were pulled from the resultCache. It was quite finicky, and modifications to how the download functioned was dangerous: if these were not kept in check, it was possible that the doneQueue would blow up.

It was also quite resource intensive, where a lot of counting and cross-checking was going on between the various pools and queues.

This has now been reworked, so that

  • the resultCache maintains (like previously) a slice of *fetchResults, with a length of blockCacheLimit * 2.
  • the resultCache also knows that it should only consider the first 75% of available slots to be up for filling. Thus, when a reserve request comes in (we want do give a task to a peer), the resultCache checks if the proposed download-task is in that priority segment. Otherwise, it flags for throttling.
  • Once the results are fetched for processing, and removed from the internal slice, the priority segment moves organically, and new data become eligible for fetching.

This means I could drop all donePool thingies, which simplified things a bit.

Concurrency

Previously, the queue maintained one lock to rule them all. Now, the resultCache has it's own lock, and can handle concurrency internally. This means that body and receipt fetch/delivery can happen simultaneously, and also that verification (sha:ing) of the bodies/receipts doesn't block other threads waiting for the lock.

Previously, I think it was kind of racy when setting the Pending on the fetchResult. This has been fixed.

Tests

The downloader tests failed quite often; when receipts are added in the backend, the headers (ownHeaders) were deleted and moved into ancientHeaders. If this happened quickly enough, the next batch of headers errored with unknown parent. This has been fixed so the backend also queries ancientHeaders for header existence.

Minor changes

  • Set the incoming response time earlier in the flow, so it doesn't have to wait for obtaining locks before setting it. Should make the rtt measurements a bit more closer to the thuth.
  • the idle check used some bubble sort algo, replaced
  • the fetcher did a lot of useless work, iterating in the block filter (every block) and calculating hashes over and over again. This was simplified

@holiman holiman marked this pull request as ready for review June 26, 2020 09:04
@karalabe
Copy link
Member

Linter says no

Copy link
Member

Choose a reason for hiding this comment

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

In my PR from today I broke out of the for loop (i.e. if it's throttled, there's nothing for other peers either), seems cleaner than continuing. In this PR won't request be nil in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be that there were a couple of items before throttling hit. So it will continue, and on next iteration it will exit.

downloader, fetcher: throttle-metrics, fetcher filter improvements, standalone resultcache

downloader: more accurate deliverytime calculation, less mem overhead in state requests

downloader/queue: increase underlying buffer of results, new throttle mechanism

eth/downloader: updates to tests

eth/downloader: fix up some review concerns

eth/downloader/queue: minor fixes

eth/downloader: minor fixes after review call

eth/downloader: testcases for queue.go

eth/downloader: minor change, don't set progress unless progress...

eth/downloader: fix flaw which prevented useless peers from being dropped

eth/downloader: try to fix tests

eth/downloader: verify non-deliveries against advertised remote head

eth/downloader: fix flaw with checking closed-status causing hang

eth/downloader: hashing avoidance

eth/downloader: review concerns + simplify resultcache and queue

eth/downloader: add back some locks, address review concerns

downloader/queue: fix remaining lock flaw
@karalabe karalabe force-pushed the refactor_downloader_2 branch from a9cbd53 to 85c308d Compare July 23, 2020 06:34
@karalabe karalabe force-pushed the refactor_downloader_2 branch from 85c308d to dec5893 Compare July 23, 2020 07:11
@karalabe karalabe added this to the 1.9.18 milestone Jul 23, 2020
@karalabe karalabe merged commit 1059221 into ethereum:master Jul 24, 2020
@KimKyungup KimKyungup mentioned this pull request Dec 2, 2020
9 tasks
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
* eth/downloader: refactor downloader + queue

downloader, fetcher: throttle-metrics, fetcher filter improvements, standalone resultcache

downloader: more accurate deliverytime calculation, less mem overhead in state requests

downloader/queue: increase underlying buffer of results, new throttle mechanism

eth/downloader: updates to tests

eth/downloader: fix up some review concerns

eth/downloader/queue: minor fixes

eth/downloader: minor fixes after review call

eth/downloader: testcases for queue.go

eth/downloader: minor change, don't set progress unless progress...

eth/downloader: fix flaw which prevented useless peers from being dropped

eth/downloader: try to fix tests

eth/downloader: verify non-deliveries against advertised remote head

eth/downloader: fix flaw with checking closed-status causing hang

eth/downloader: hashing avoidance

eth/downloader: review concerns + simplify resultcache and queue

eth/downloader: add back some locks, address review concerns

downloader/queue: fix remaining lock flaw

* eth/downloader: nitpick fixes

* eth/downloader: remove the *2*3/4 throttling threshold dance

* eth/downloader: print correct throttle threshold in stats

Co-authored-by: Péter Szilágyi <peterke@gmail.com>
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Jun 9, 2025
* eth/downloader: refactor downloader + queue

downloader, fetcher: throttle-metrics, fetcher filter improvements, standalone resultcache

downloader: more accurate deliverytime calculation, less mem overhead in state requests

downloader/queue: increase underlying buffer of results, new throttle mechanism

eth/downloader: updates to tests

eth/downloader: fix up some review concerns

eth/downloader/queue: minor fixes

eth/downloader: minor fixes after review call

eth/downloader: testcases for queue.go

eth/downloader: minor change, don't set progress unless progress...

eth/downloader: fix flaw which prevented useless peers from being dropped

eth/downloader: try to fix tests

eth/downloader: verify non-deliveries against advertised remote head

eth/downloader: fix flaw with checking closed-status causing hang

eth/downloader: hashing avoidance

eth/downloader: review concerns + simplify resultcache and queue

eth/downloader: add back some locks, address review concerns

downloader/queue: fix remaining lock flaw

* eth/downloader: nitpick fixes

* eth/downloader: remove the *2*3/4 throttling threshold dance

* eth/downloader: print correct throttle threshold in stats

Co-authored-by: Péter Szilágyi <peterke@gmail.com>
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Jun 17, 2025
* eth/downloader: refactor downloader + queue

downloader, fetcher: throttle-metrics, fetcher filter improvements, standalone resultcache

downloader: more accurate deliverytime calculation, less mem overhead in state requests

downloader/queue: increase underlying buffer of results, new throttle mechanism

eth/downloader: updates to tests

eth/downloader: fix up some review concerns

eth/downloader/queue: minor fixes

eth/downloader: minor fixes after review call

eth/downloader: testcases for queue.go

eth/downloader: minor change, don't set progress unless progress...

eth/downloader: fix flaw which prevented useless peers from being dropped

eth/downloader: try to fix tests

eth/downloader: verify non-deliveries against advertised remote head

eth/downloader: fix flaw with checking closed-status causing hang

eth/downloader: hashing avoidance

eth/downloader: review concerns + simplify resultcache and queue

eth/downloader: add back some locks, address review concerns

downloader/queue: fix remaining lock flaw

* eth/downloader: nitpick fixes

* eth/downloader: remove the *2*3/4 throttling threshold dance

* eth/downloader: print correct throttle threshold in stats

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Péter Szilágyi <peterke@gmail.com>
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.

2 participants