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

chore: Add performance benchmark to diff-sequences package #7603

Merged
merged 7 commits into from
Jan 20, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

Compare improved diff-sequences package to baseline diff package.

  • Added perf/index.js file
  • Added perf/example.md file which contains copied result of a run
  • Added a command to "scripts" in package.json file
  • Added benchmark and diff as devDependencies

Because allocating and freeing of temporary objects is the root of the performance problem, the tests call global.gc() before every test cycle, so make sure to run node with --expose-gc option!

Above 2000 items, the benchmark package can’t keep the relative mean error below its target of 1%

For example, notice in next to last row of example.md that an outlier low 0.0083 ratio corresponds to outlier high 3.60% baseline rme: an inaccurate high denominator causes low ratio.

Your critique is always welcome and especially because perf benchmark is new for me :)

P.S. I added /* eslint import/no-extraneous-dependencies: "off" */ because the rule demanded that benchmark and diff become dependencies instead of devDependencies

Test plan

To achieve 1% relative mean error, run the benchmark:

  • immediately after restart
  • with 100% battery charge
  • not connected to network

I ran it with node 10.15.0 not via yarn in subdirectory: node --expose-gc perf/index.js

See most values of ratio are between 0.05 and 0.15

The most relevant test for catastrophic performance problems is insert 80% that is, after every 2 expected items, insert 8 unexpected items:

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is awesome, great job!

packages/diff-sequences/perf/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Would be cool to include some tests that favor longer lines instead of more lines and see how that compares. Looks good:)

@SimenB
Copy link
Member

SimenB commented Jan 20, 2019

@pedrottimark thoughts on @thymikee's comment?

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Jan 20, 2019

Yes, it is a great point to add more realistic tests.

@thymikee review comments always stretch my thinking: it hit me just now that an additional benchmark is compare diff-sequences to the amazing diff-match-patch for strings (array of characters) and see how much it cost to gain the flexibility of callback functions. Not realistic for me to write that test before Jest 24 though.

For y’all info, a long term goal to solve edge cases in report when assertion fails is for expect package to compute the differences as a recursive array tuple data structure (so type information isn’t lost by premature serialization) and therefore diff becomes more tactical (and catastrophic edge cases we solved will not occur). I have written some rough prototype code and the next step is for me to open an issue so we can discuss and develop some realistic tests of both clarity and performance.

@thymikee
Copy link
Collaborator

Not realistic for me to write that test before Jest 24 though

Don't bother it now, it's just a nice-to-have for somewhere later. If you have any personal to-do list, it's worth to put it there, or create an issue to not forget about a followup :)

@pedrottimark pedrottimark merged commit 6c3d998 into jestjs:master Jan 20, 2019
@pedrottimark pedrottimark deleted the benchmark-diff-sequences branch January 20, 2019 20:27
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
…7603)

* chore: Add performance benchmark to diff-sequences

* Change require to refer to build directory

* Update CHANGELOG.md

* Add Facebook copyright header to index.js file

* Replace process.stdout.write with console.log

* Add link to 7627 as a way to restart CI :)
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants