Use binary search in the prioritized task executor #129
Conversation
|
Added a lock. This is the first time I'm using this semaphore stuff, so please carefully review it 😄 Should fix the tests first though. |
src/prioritizedTaskExecutor.ts
Outdated
| if (this.currentPoolSize < this.maxPoolSize) { | ||
| this.currentPoolSize++ | ||
| fn(() => { | ||
| fn(async () => { | ||
| await this.lock.acquire() |
There was a problem hiding this comment.
This looks quite dangerous. If this callback gets executed and await right away you'd get into a deadlock, won't you?
That being said, I don't know much about this part of MPT. I was just casually looking at this because of your message on discord.
There was a problem hiding this comment.
I have to update it because the tests are failing, but I don't think this will deadlock: if it's locked it means that some other Promise is running, which will unlock it at some point.
There was a problem hiding this comment.
Never mind you are right 😅
jochem-brouwer
force-pushed
the
priority-queue-optimized
branch
from
8e84352 to
9b5bbf8
Compare
|
Testing it on a Geth DB seems to hang, so need to battle-test this first. |
|
Tested this on the mainnet DB of Geth. Surprisingly, this did not result in a big performance increase, my original task which took about 400 seconds went down to 380 seconds. The code is rather complex currently and a bit all-over-the-place, will clean it up. Flamegraph of above's task with ~25k states: Used console.log to track progress. |
|
Ready for review |
|
Let's maybe wait for the benchmark work from @ryanio to get a bit clearer picture here. This is a lot of new code, I would rather want to see if it is worth to add that much of new complexity before merging in. |
|
Yeah I agree, the performance increase seems (empirically) to be not that exciting and this code is rather complex. I am also not very confident at this point since I do not understand why the code fails to work if I put the |
In the Prioritized Task Executor originally every time when a task finishes, the priority list is sorted again. This is not optimal as we can do this directly when we insert it. For instance,
_walkTrieis expected to get a gigantic queue at some point if we have a large MPT, which means we might be sorting this large array every time MPT finds a new item.There is possibly one problem here: a task might finish while the insertion algorithm is ran. I think we need a lock mechanism here? Suggestions?