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

Remove the DOM nodes recycling pool #2122

Merged
merged 1 commit into from Apr 16, 2018

Conversation

Projects
None yet
3 participants
@pygy
Copy link
Member

pygy commented Apr 15, 2018

This is an alternative to #1675 which removes the recycling pool altogether. I couldn't find the time to properly benchmark the change, but other libs (among the fastest ones) have dropped that approach since it didn't bring them any significant advantage.

The pool makes the whole render/render.js more complex. Having it out for now should make other needed changes easier (like, the longest increasing subsequence code to be lifted from Ivi).

I've left the tests in place (with some modification to assert that nodes are not recycled). If we decide to commit to this approach in the long run we can remove them too (I'd rather keep them in for now).

Edit: like #1675, this also fixes #1653 and #2023 that were purely caused by the existence of the pool.

Edit2: I also removed the corresponding benchmark.

@pygy pygy requested a review from isiahmeadows as a code owner Apr 15, 2018

@pygy pygy force-pushed the pygy:nix-the-pool-2018-4 branch from f4af5af to 997d950 Apr 15, 2018

@pygy pygy requested a review from tivac as a code owner Apr 15, 2018

@pygy pygy force-pushed the pygy:nix-the-pool-2018-4 branch from 997d950 to 142b8da Apr 15, 2018

@pygy pygy force-pushed the pygy:nix-the-pool-2018-4 branch from 142b8da to 21e3dbb Apr 15, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 15, 2018

@pygy I'll note that even though it technically is faster if you do it right and are already fast enough that node allocation becomes a real bottleneck (Inferno saw dramatic 2-3x increases, but Mithril might see a 20% increase), it complicates code and makes user behavior much less predictable. That's the main reason libraries don't like to keep it (Inferno silently dropped it for v4), and the few that do offer it usually keep it disabled by default.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 16, 2018

Ran the benchmarks 3x in Chrome 65

next

rerender identical vnode x 4,837,240 ops/sec ±2.45% (59 runs sampled)
rerender same tree x 101,246 ops/sec ±1.95% (55 runs sampled)
construct large VDOM tree x 28,710 ops/sec ±4.08% (54 runs sampled)
mutate styles/properties x 12,486 ops/sec ±2.51% (52 runs sampled)
repeated trees (recycling) x 484 ops/sec ±25.05% (16 runs sampled)
repeated trees (no recycling) x 279 ops/sec ±9.83% (38 runs sampled)
Completed perf tests in 75522ms

rerender identical vnode x 4,460,507 ops/sec ±7.01% (50 runs sampled)
rerender same tree x 97,156 ops/sec ±1.89% (52 runs sampled)
construct large VDOM tree x 29,245 ops/sec ±2.94% (57 runs sampled)
mutate styles/properties x 11,632 ops/sec ±5.87% (39 runs sampled)
repeated trees (recycling) x 639 ops/sec ±22.92% (20 runs sampled)
repeated trees (no recycling) x 343 ops/sec ±6.35% (40 runs sampled)
Completed perf tests in 82824ms

rerender identical vnode x 4,648,130 ops/sec ±3.35% (47 runs sampled)
rerender same tree x 93,917 ops/sec ±5.38% (49 runs sampled)
construct large VDOM tree x 27,968 ops/sec ±4.08% (55 runs sampled)
mutate styles/properties x 11,679 ops/sec ±4.25% (41 runs sampled)
repeated trees (recycling) x 652 ops/sec ±22.03% (19 runs sampled)
repeated trees (no recycling) x 437 ops/sec ±18.38% (19 runs sampled)
Completed perf tests in 81769ms

PR

rerender identical vnode x 4,856,641 ops/sec ±2.08% (53 runs sampled)
rerender same tree x 97,837 ops/sec ±3.54% (53 runs sampled)
construct large VDOM tree x 28,412 ops/sec ±3.47% (54 runs sampled)
mutate styles/properties x 10,819 ops/sec ±6.05% (45 runs sampled)
repeated trees x 477 ops/sec ±26.37% (15 runs sampled)
Completed perf tests in 70034ms

rerender identical vnode x 4,303,089 ops/sec ±8.65% (47 runs sampled)
rerender same tree x 95,311 ops/sec ±2.59% (49 runs sampled)
construct large VDOM tree x 26,208 ops/sec ±8.01% (50 runs sampled)
mutate styles/properties x 11,648 ops/sec ±3.77% (51 runs sampled)
repeated trees x 622 ops/sec ±25.47% (21 runs sampled)
Completed perf tests in 71354ms

rerender identical vnode x 4,885,673 ops/sec ±2.27% (51 runs sampled)
rerender same tree x 102,537 ops/sec ±1.54% (50 runs sampled)
construct large VDOM tree x 28,870 ops/sec ±4.22% (53 runs sampled)
mutate styles/properties x 12,897 ops/sec ±1.15% (51 runs sampled)
repeated trees x 654 ops/sec ±24.52% (21 runs sampled)
Completed perf tests in 67574ms
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 16, 2018

Thanks @tivac

Let's merge this!

@pygy pygy merged commit 203df39 into MithrilJS:next Apr 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment