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

Use .pop() to flush rerenderQueue instead of .shift() #4630

Closed
wants to merge 2 commits into from

Conversation

jviide
Copy link
Contributor

@jviide jviide commented Jan 11, 2025

This pull request slightly modifies how rerenderQueue is flushed.

The idea is to use .pop() to flush the rerender queue instead of using .shift(). This also requires inverting the depth sorting comparison function to keep the queue in reverse order.

The rerender queue is an array that was previously emptied by calling the array's .shift() method repeatedly. In v8 the .shift() operation appears to O(n) time cost, based on looking at the relevant parts of v8's source code and a relevant open issue. Therefore, when .shift() is called repeatedly for a large array the aggregate time spent seems to grow faster than linearly.

In contrast the .pop() method of arrays seems to behave in O(1) way, at least when amortized over many operations. Therefor calling it repeatedly tends to take a linear-ish time in aggregate.

The overall performance effect of these changes is likely very small in the common case, as these changes address only the overhead from the rerender queue management. Usually everything else, like the queued rerenders themselves, take much more time. Still, these changes aim to avoid the queue management becoming a bottleneck in extreme cases by avoiding O(n^2) behavior when hundreds or thousands rerenders are getting queued simultaneously.

The performance effect seems to be a wash when the queue stays relatively small, with the exception when the queue length is 1. There is now an optimization that skips sorting queues of length < 2.

@coveralls
Copy link

coveralls commented Jan 11, 2025

Coverage Status

coverage: 99.618% (+0.001%) from 99.617%
when pulling f6a1a8f on jviide:shift-to-pop
into 28a937c on preactjs:main.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Genius!

@jviide
Copy link
Contributor Author

jviide commented Jan 18, 2025

After further thought, the .revese().sort(...) approach doesn't keep the queue insertion order in all situations. After the first reverse the queued components on the same depth will be processed in the correct order, but the next potential .reverse() (if the queue is modified during processing) causes them go out of order again. Going to close this for now, but the small refactoring that allows skipping the initial .sort() call is now included in #4637.

@jviide jviide closed this Jan 18, 2025
@jviide jviide deleted the shift-to-pop branch January 18, 2025 18:07
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.

3 participants