I'm gonna put this back into draft while I try to address your concerns, @holiman . If I don't think I can get to something cleaner then I'll close it.

Thank you for the thorough review and for the pushback @holiman—both are appreciated and made me completely rethink this. I won't object if you still want to reject the PR based on the slippery terrain, but I at least wanted to offer something better.

There's now only a small (non-blocking) "fan-in" go routine and a channel range over the fanned in work. Each element of the pipeline is now self-contained so it's easy to reason about. To me, the wake channel suggested an anti-pattern and it's now unnecessary due to native channel semantics.

I'll move this out of draft once I've thought it through a little more, but if you still want to close the PR then please do so I don't waste time.

I also don't like the nested selects. IMO it's un-intuitive and hard to reason about.

Removed

This is now blocking. ... I'm guessing it was unintentional.

Yes, it was.

In redesigning following holiman's review, I found a race condition in the old implementation that this also fixes. It can be demonstrated by running TestSchdeulerTerminationRaceCondition against master.

I found a race condition in the old implementation that this also fixes.

IIUC, this race happens if new tasks are added while we're closing the thing. However, as far as I know, we add tasks while executing transactions in the block, and only close it after we're done with the execution.

So, from a unit-testing perspective, it's a race, but given the context it's being used in, it's fine. Does this sound correct?

@holiman I have rewritten the implementation a bit and the nested select has been removed. https://github.com/rjl493456442/go-ethereum/tree/arr4n/lockless-trie-subfetcher

I am a bit hesitant. You are right the we will never invoke terminate and schedule at the same time. The existing data race is not a real problem for us. But as Felix said that using channel and lock is a bit wonky.

I would like to get some preliminary feedback about my changes first and then to decide whether changing the code is worthwhile.

So, from a unit-testing perspective, it's a race, but given the context it's being used in, it's fine. Does this sound correct?

Yup, as you both think that scheduling and termination won't coincide then I agree that the race is theoretical. The test can definitely be excluded however I do think the rest of the changes benefit the code w.r.t clarity and general correctness of implementation vs expectation (that scheduled work is performed and sf.terminate(false) blocks accordingly).

But as Felix said that using channel and lock is a bit wonky.

This is what motivated me to try find something cleaner. My first attempt wasn't great but I think the latest version (sans test) removes as much complexity as possible.

@holiman I have rewritten the implementation a bit and the nested select has been removed. https://github.com/rjl493456442/go-ethereum/tree/arr4n/lockless-trie-subfetcher

@rjl493456442 I see that your changes were on top of my rewrite that also removed the nested select. Was there something in particular that you wanted to improve on?

I would like to get some preliminary feedback about my changes first and then to decide whether changing the code is worthwhile.

I find it a little difficult to reason about the replacement and checking of the done channel plus the multiple spawning points of sf.process(). IIUC the goal is to have only one background processor at a time, which I think is simpler with a channel range because it allows the done channel (and its case) to be removed.

Was there something in particular that you wanted to improve on?

Yep, nested select looks a bit hacky to me.

IIUC the goal is to have only one background processor at a time

Yes it's true. At most one process be spawned at the same time.

The done channel is used to not block the task buffering even when the process is running.

So, let's imagine we invoke schedule 1000 times very quickly. On master,

• All tasks are appended to the sf.tasks. The main loop is woken once, then maybe couple of times more to "re-arm" the tasks.
• All tasks, once they are appended to the sf.tasks are sequential. So, if the 1000 invokations are adding one task each, 1 to 1000, then they get executed in strict order (just mentioning this attribute, but I will admit that I don't think strict ordering of tasks is necessary in this case)

As far as I can tell of the current state of this PR, in @ARR4N 's version,

• The main loop will greedily pop items off the sf.tasks, and spin up goroutines. So it will have 1000 goroutines trying to deliver their tasks on work.
• Once the goroutines are trying to deliver their tasks, they lose ordering (between batches, not internally. But in the exampe of 1000 invocations with 1 task each, we lose all ordering).

So IMO that exhibits two behavioural changes that seem to be unintentional side-effects. Neither of those behavioural changes are good, IMO.

As for @rjl493456442 's changes

• The tasks will be appended to, just like on master: the difference being that the appending happens within the main loop, instead of in the callsite. So avoiding the need to have a lock.
• Ordering is maintained.
• I do think this version is slightly cleaner than current master: without the lock, and with the processing moved to a standalone method.
• On master, it's trivially obvious that adding a subfetcher task cannot block. Not so with this version: adding a task requires the main loop to receive it. Which should be a fairly quick operation, though.
• This version (well, both) will have a lot more short-lived goroutines than master. Which might be fine.

IMO the changes in this PR are not acceptable, so I'll close this PR.

The changes in @rjl493456442 's version are, I think, possibly acceptable. I'd rather discuss those in a separate PR, if you want to pursue it.

IMO the changes in this PR are not acceptable, so I'll close this PR.

Thanks for considering it and for the very thorough reviews.

Yep, nested select looks a bit hacky to me.

I think you might have mixed up the original nested with the updated, non-nested version. I'm happy with @holiman's decision given the rationale so sharing this only in case you want to see an alternate approach, @rjl493456442.

I think you might have mixed up the original nested with the updated, non-nested version.

I think he referrred to this:

Such towers are pretty non-intuitive, IMO.