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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perf testing #1789

Merged
merged 16 commits into from Apr 13, 2017

Conversation

Projects
None yet
5 participants
@tivac
Copy link
Member

tivac commented Apr 8, 2017

This is based on preact's perf test because that seemed like a reasonable place to start. I tried to pick some reasonable thresholds that passed muster on Travis but they're more of a guess than anything.

Notably this does not measure m.redraw() perf, only m.render(). It's simpler that way and we can introduce more complexity later.

Currently doing 5 runs of each benchmarking script, takes about 12s to run locally. On Travis it'll run after all the tests/linting completes so it shouldn't mess up too much in terms of slower builds.

npm run perf or you can open index.html in a browser.

Don't actually need a review from everyone, but it's a good notification vector 馃槃

@tivac tivac added the enhancement label Apr 8, 2017

@tivac tivac requested review from pygy , lhorie and isiahmeadows Apr 8, 2017

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

Generally looks good to me. Great work, BTW! 馃槃

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 8, 2017

@tivac I'll look at this later today. Looks good at a cursory glance.

@orbitbot

This comment has been minimized.

Copy link
Contributor

orbitbot commented Apr 8, 2017

One idea to avoid too much guesswork with perf value ranges would be to have this be passively checking updates for a while, eg. perhaps a month of commits before trying to enforce the values. Or alternatively, some scripting could be done to check for values historically (this might also be interesting from other POWs and perhaps suggest some further improvements), which ofc requires a bit of more work. I'm assuming this is supposed to be an early-warning system or perhaps even fail PRs if they noticeably degrade perf.

@pygy
Copy link
Member

pygy left a comment

That's an excellent start. I'm going to assume that you/the Preact folks did their homework for the core benchmark, loop and tick business...

A big 馃憤 once points 1 and 3 below are addressed (point 2 can wait).


benchmark(
function () {
m.render(scratch, vdom);

This comment has been minimized.

@pygy

pygy Apr 8, 2017

Member

Are you intentionally rendering a cached node? Given its size, something tells me it is an oversight...

The first render here will set up the DOM, and the following ones are ~noops.

This comment has been minimized.

@tivac

tivac Apr 9, 2017

Member

That's intended, it's testing maximum throughput for diffing when nothing has changed. A baseline if you will.

This comment has been minimized.

@pygy

pygy Apr 9, 2017

Member

But then why the large tree? updateNode will bail out when it sees the roots are identical... Or is it to detect regressions where we would stop bailing out early? If so, 馃憤

This comment has been minimized.

@pygy

pygy Apr 9, 2017

Member

... but then, I think we have tests that verify that the update phase doesn't run on cached nodes... well, I guess some redundance can't hurt.

function () {
m.render(scratch, m(Parent, {child : Root}));
m.render(scratch, m(Parent, {child : Empty}));
},

This comment has been minimized.

@pygy

pygy Apr 8, 2017

Member

You may want to duplicate this test, with two m.render(scratch, m(Parent, {child : Empty})); after m.render(scratch, m(Parent, {child : Root})); in order to nullify the pool (if the pool is modified to last longer than one redraw we'd have to update this accordingly).

Also, why the Parent gymnastics rather than m.render(scratch, [m(Root)]);m.render(scratch, []) ?

This comment has been minimized.

@tivac

tivac Apr 9, 2017

Member

I don't totally understand your comment about the pool. Can you expand on that a bit?

Parent stuff is ported directly from the preact tests, I assume it's a weird react-ism. I'll remove and make it a bit more mithril-y.

This comment has been minimized.

@pygy

pygy Apr 9, 2017

Member

The nodes in the recycling pool are discarded after one redraw where they were not used:

http://jsbin.com/giqudojumo/edit?html,js,console

If you render [m('p')]) then [] then [m('p')], Mithril recycles the original DOM node because it was in the pool. If you do [m('p')]) => [] => [] => [m('p')], the second empty list doesn't have the original <p> in its pool (it has no pool actually) and Mithril creates a new DOM node.

The Parent dance I asked you to change was actually busting the pool (which only works in list context), so my comment was partly misguided... But it would be interesting to have it both pooled and unpooled anyway.

{"class": get(classes, index), "data-index": index, title: index.toString(36)},
m("input", {type: "checkbox", checked: index % 3 == 0}),
m("input", {value: "test " + (Math.floor(index / 4)), disabled: index % 10 ? null : true}),
m("div", {"class": get(classes, index * 10)},

This comment has been minimized.

@pygy

pygy Apr 8, 2017

Member

(index * 10) % 5 === 0, class is always foo

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

I just noticed...

Apart from @pygy's review, you also need to drop all these semicolons. The rest of the code base doesn't use them, and ESLint checks for this, too.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 9, 2017

@isiahmeadows Interesting, npm run lint isn't complaining about that. I removed a bunch manually but didn't even notice the rest because I'm 100% a semicolons required guy.

Why doesn't .eslintrc.js have

"semi": [
	"error",
	"never"
],

in it?

tivac added some commits Apr 9, 2017

refactor: tweak logic for actual class variation
And threshold, now that it's slower
@pygy

pygy approved these changes Apr 9, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 9, 2017

@tivac

I removed a bunch manually but didn't even notice the rest because I'm 100% a semicolons required guy.

You're good. I only notice semicolons quickly because I personally prefer ASI and set ESLint to check that in my personal projects.

Why doesn't .eslintrc.js have [...] in it?

Because it needs fixed to enforce the existing style throughout.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 9, 2017

@pygy I'll do my best to add a test for pooled vs unpooled nodes. No promises I'll get it right though.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 9, 2017

@tivac

This should do (just after the previous benchmark() call):

		benchmark(
			function () {
				m.render(scratch, [m(Root)])
				m.render(scratch, [])
				m.render(scratch, [])
			},
			verify("repeated trees (pool disabled)", 3500, done)
		)
@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 10, 2017

@pygy split it up in a slightly different way, but now there's a test for pooling vs not-pooling. Makes very little difference for me locally.

$ npm run perf

rerender without changes: 3174787/s (3 ticks, 60% of threshold)
repeated trees (recycled): 4362/s (2443 ticks, 69% of threshold)
repeated trees (no recycling): 4478/s (2135 ticks, 61% of threshold)
construct large VDOM tree: 5150/s (2016 ticks, 80% of threshold)
mutate styles/properties: 52257/s (207 ticks, 59% of threshold)
10 assertions completed in 15381ms, of which 0 failed

@isiahmeadows are we good now that the semicolons have been dropped?

@lhorie

lhorie approved these changes Apr 10, 2017

Copy link
Member

lhorie left a comment

Probably should use benchmark.js, but other than that, seems reasonable

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 10, 2017

@lhorie I was trying to avoid adding even more external devDependencies to the repo since IIRC you don't want a bunch, but if that restriction is lifted I have some plans.

馃槂

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 10, 2017

Wooo.... plans... now I'm curious :-)

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 11, 2017

@tivac

are we good now that the semicolons have been dropped?

Yes. I was just a little behind in approving it.

I was trying to avoid adding even more external devDependencies to the repo since IIRC you don't want a bunch, but if that restriction is lifted I have some plans.

And you now have my curiousity...could...we get a sneek preview...? 馃槈

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 12, 2017

Nothing concrete enough yet, I'll try to write up an issue or PR or something. I'll port this to benchmark.js tonight.

tivac added some commits Apr 12, 2017

refactor: use benchmark.js, remove ospec
Also no longer sets thresholds or failure states, it's purely
informational
refactor: remove browser support
Benchmark requires lodash and I don't want to deal with figuring out how
to load that into the browser atm
@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 12, 2017

@lhorie reworked using benchmark.js

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 13, 2017

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Apr 13, 2017

Ugh

tivac added some commits Apr 13, 2017

@tivac tivac merged commit 9a55a29 into MithrilJS:next Apr 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tivac tivac deleted the tivac:perf branch Apr 13, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 13, 2017

馃帀 w00t :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment