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

Avoid using shift in flush #4356

Merged
merged 2 commits into from
Feb 8, 2020
Merged

Avoid using shift in flush #4356

merged 2 commits into from
Feb 8, 2020

Conversation

kesne
Copy link
Contributor

@kesne kesne commented Feb 3, 2020

We noticed that there was a large amount of memory being created as part of the flush method, which seems to be due to some array optimizations in old JS engines. This seems to be easily fixed by changing to a simple for loop instead of a while loop that modifies the array during iteration. This is the same pattern that is used for the render_callbacks.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

We noticed that there was a large amount of memory being created as part of the flush method, which seems to be due to some array optimizations in old JS engines. This seems to be easily fixed by changing to a simple for loop instead of a while loop that modifies the array during iteration. This is the same pattern that is used for the `render_callbacks`.
@ThePrimeagen
Copy link

This is great, shift is extremely expensive even in modern engines. cc trentm/node-bunyan#279

But, I will have to say @kesne , this i += 1... makes me feel like I don't even know you.

@Conduitry
Copy link
Member

The test failure looks like a legitimate one. This apparently causes an infinite loop with custom elements.

@kesne
Copy link
Contributor Author

kesne commented Feb 3, 2020

@Conduitry It appears that the failed tests are due to the internal flush() method getting called infinitely, as property updates will end up calling flush(). This worked previously because the array was mutated, so the nested flush() calls wouldn't break.

It seems like the easiest resolution is to only allow one copy of the flush() function running at a time. I tested this change it seems like all tests pass after implementing it. This also seems more ideal from a memory perspective, as it should minimize callstack depth.

@Conduitry Conduitry merged commit 4b04b16 into sveltejs:master Feb 8, 2020
@olivierchatry
Copy link

Just wondering, would it be better to do the same thing ( replace while with a for ) for :

		while (binding_callbacks.length) binding_callbacks.pop()();

Not sure how JS engine handle memory allocation for array, but in any case, seems better to have a for loop in there, and reset the size at the end like you did for this optims ?

Also, is dirty_components can change while executing the for loop ? If it is constant, than, if I remember correctly, using .length have a cost as well ( better store it in a const before the for loop ). Small optims probably, but a gain is a gain!

@kesne
Copy link
Contributor Author

kesne commented Feb 8, 2020

@olivierchatry I'm unsure about the while case, it doesn't seem like there's any cases where it's causing problematic amounts of memory.

As for dirty_components changing, it can change while being iterated, so it is not possible to hoist it to a constant. In general, engines cache these frequently-accessed properties so that the lookup is not expensive.

@olivierchatry
Copy link

Ho, sorry, I was not clear, I was not talking about memory directly, but more about CPU usage, I'm wondering if it is faster to do a for then a clear, than a while with a pop.

Thanks for the heads up, I think you are very correct, it's minor optimization anyway, and I'm not even sure it would change anything.

@olivierchatry
Copy link

Actually, seems pop is faster than length = 0, wild guess, pop does not deallocate memory, just the internal "index", while length = 0 actually deallocate ( but would have to check the code ).

Anyway sorry for the thread, I'm reading the inside of svelt to learn it, and this PR seemed cool. Thanks a lot !

@Conduitry
Copy link
Member

I merged this PR earlier today - Are there other changes that people think should happen before the next release? I don't have strong opinions or much familiarity with the performance differences between the various ways of doing this.

@kesne
Copy link
Contributor Author

kesne commented Feb 9, 2020

I don’t think so. This should be fine to release as-is. This solves a specific de-opt case whereas the discussion here is more around further ways to optimize.

@Conduitry
Copy link
Member

3.18.2 has been released with this change, thanks!

@olivierchatry
Copy link

olivierchatry commented Feb 9, 2020 via email

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.

4 participants