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

render: fix perf regression introduced by #1918 #2052

Merged
merged 1 commit into from Dec 9, 2017

Conversation

Projects
None yet
4 participants
@pygy
Copy link
Member

pygy commented Dec 8, 2017

I wanted to make sure that the changes I'm making to updateNodes don't have a negative impact on benchmarks and noticed a stark perf regression in the next branch when compared to v1.1.6. It turns out that checking parentNode happens to be toxic. Since #1916 was about select/option, I've limited the check to option elements.

How Has This Been Tested?

as little as #1918 since the mocks still don't support the correct API.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@pygy pygy requested a review from isiahmeadows as a code owner Dec 8, 2017

@pygy pygy force-pushed the pygy:fix-perf-regression branch from 9eda8f1 to 1782fa8 Dec 8, 2017

@pygy pygy requested a review from tivac as a code owner Dec 8, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 8, 2017

@robinchew does this look correct to you?

@tivac

tivac approved these changes Dec 8, 2017

Copy link
Member

tivac left a comment

Good catch!

@isiahmeadows isiahmeadows merged commit 1ad9f84 into MithrilJS:next Dec 9, 2017

1 check passed

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

This comment has been minimized.

Copy link
Contributor

robinchew commented Dec 9, 2017

How do you check for performance regression?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Dec 9, 2017

We have a builtin npm run perf and I was actually using https://github.com/krausest/js-framework-benchmark. I duplicated the Mithril benchmark and made it depend on my local git version of Mithril rather than the NPM version. The README of the project explains how to do that.

I used git bisect to identify the problem commit.

From the branch HEAD:

git bisect start
git bisect bad # mark the current commit as bad
git bisect good v1.1.5 # mark the last known good commit

At that point, git bisect will have checked out the commit in the middle of the section you want to investigate.
Then build the benchmark app, run the benchmarks. If the results are fast, git bisect good otherwise, git bisect bad. That will mark the current commit accordingly, and once again checkout the commit in the midddle of the unknown section.

HEAD // bad
-1
-2
-3
-4 <== test here... it's fast
-5
-6
-7 //good

Mark -4 ad good: git bisect good

HEAD // bad
-1
-2 // test here... it's slow
-3
-4 // good
-5
-6
-7 //good

Mark -2 as bad: git bisect bad

HEAD // bad
-1
-2 // bad
-3 // test here
-4 // good
-5
-6
-7 //good

Repeat until you've identified the first bad commit. Git bisect automatically moves the cursor for you.

I focused on the worse regression because running the benchmarks reliably requires me to exit every program and leave the computer alone. It turns out there are other problems.

I'm not faulting you here BTW @robinchew, your PR passed review. I didn't expect such a simple access to be that expensive.

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 12, 2018

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