perf: uses setImmediate() implementation from tiny-set-immediate#109
perf: uses setImmediate() implementation from tiny-set-immediate#109rubensworks merged 3 commits intoRubenVerborgh:mainfrom
Conversation
…performant macrotask scheduling regardless of whether setImmediate() is natively available
rubensworks
left a comment
rubensworks
left a comment
There was a problem hiding this comment.
Are you seeing any (negative) impact when running the perf script?
From From this PR: |
| }); | ||
|
|
||
| describe('a task scheduler when setImmediate exists', () => { | ||
| describe('a task scheduler using an externally-provided setImmediate() implementation', () => { |
There was a problem hiding this comment.
| describe('a task scheduler using an externally-provided setImmediate() implementation', () => { | |
| describe('a task scheduler with default setImmediate implementation', () => { |
| describe('a task scheduler when setImmediate exists', () => { | ||
| describe('a task scheduler using an externally-provided setImmediate() implementation', () => { | ||
| const backups = {}; | ||
| let setImmediate = sinon.spy(); |
There was a problem hiding this comment.
just let it default here; or spy on tiny-set-immediate itself
| }); | ||
| }); | ||
|
|
||
| describe('a task scheduler when setImmediate does not exist', () => { |
There was a problem hiding this comment.
Maybe keep this test, but repurpose for custom implementation? I like that the test verifies the alternating behavior in another way.
describe('a task scheduler with custom setImmediate implementation',
|
|
||
| it('can schedule a task', done => { | ||
| const taskScheduler = createTaskScheduler(); | ||
| const taskScheduler = createTaskScheduler(setImmediate); |
There was a problem hiding this comment.
suggest to leave unchanged
|
@RubenVerborgh I couldn't find a way to spy on |
|
@rubensworks @RubenVerborgh just checkin in on this PR. I have not addressed all of the comments above as I wasn't able to find a way to spy on Insofar as I remember this should be good to go, though perhaps it'd be worth adding an explicit mention to No pressure, as always. Over the next few weeks I'll probably experiment with using |
3 participants
This PR changes the task scheduler to use an externally-provided macrotask scheduling function and defaults to using the
setImmediate()implementation fromtiny-set-immediate.This is done to work around performance issues discovered in comunica/comunica#1552 and ultimately traced back to the enormous difference between
setImmediate(), which is only available is Node.js-like environments, andsetTimeout(fn, 0), which is what the in-housescheduleMacrotaskfunction would fallback to prior to this change. The former is roughly 90x faster than the latter.