Skip to content

Adds perf tests for Map.merge. #1407

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

Merged
merged 5 commits into from
Oct 18, 2017
Merged

Adds perf tests for Map.merge. #1407

merged 5 commits into from
Oct 18, 2017

Conversation

abacaphiliac
Copy link
Contributor

Output sample:

Map > merge an object of 2
  Old:   932,047   960,634   991,030 ops/sec
  New:   978,449   983,047   987,688 ops/sec
  compare: -1 1
  diff: 2.33%
  rme: 2.19%
Map > merge an object of 8
  Old:   568,422   572,977   577,606 ops/sec
  New:   571,550   575,883   580,283 ops/sec
  compare: 0 0
  diff: 0.5%
  rme: 0.78%
Map > merge an object of 32
  Old:   251,965   254,599   257,289 ops/sec
  New:   228,904   232,267   235,731 ops/sec
  compare: 1 -1
  diff: -8.78%
  rme: 1.27%
Map > merge an object of 1024
  Old:     8,428     8,491     8,554 ops/sec
  New:     7,625     7,681     7,739 ops/sec
  compare: 1 -1
  diff: -9.54%
  rme: 0.73%
Map > mergeDeep an object of 2
  Old:   321,103   323,956   326,860 ops/sec
  New:   323,939   328,831   333,872 ops/sec
  compare: -1 1
  diff: 1.5%
  rme: 1.23%
Map > mergeDeep an object of 8
  Old:   125,157   126,434   127,736 ops/sec
  New:   124,574   126,283   128,040 ops/sec
  compare: 0 0
  diff: -0.12%
  rme: 1.2%
Map > mergeDeep an object of 32
  Old:    37,218    37,439    37,662 ops/sec
  New:    36,882    37,395    37,923 ops/sec
  compare: 0 0
  diff: -0.12%
  rme: 1.06%
Map > mergeDeep an object of 1024
  Old:       728       740       752 ops/sec
  New:       698       712       726 ops/sec
  compare: 1 -1
  diff: -3.82%
  rme: 1.8%

Full output: map-merge-perf.txt

@abacaphiliac
Copy link
Contributor Author

@leebyron please review.

Copy link
Collaborator

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for adding to these!

perf/Map.js Outdated
}

const m1 = Immutable.Map(obj);
const m2 = Immutable.Map(obj);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two are the same - there's a special path for no-op merges, so you might want to add a separate perf test where the second map contains different values

perf/Map.js Outdated
@@ -139,4 +139,39 @@ describe('Map', function () {

});

describe('merge an object', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

'merge a map'?

@abacaphiliac
Copy link
Contributor Author

@leebyron Done. Thanks!

Are you interested in me getting the perf tests running in Travis?

@abacaphiliac
Copy link
Contributor Author

@leebyron Resolved (style) conflicts.

@leebyron
Copy link
Collaborator

I think the perf tests might be too slow to run in Travis, also they're pretty sensitive the the operating environment so I think they may also be too noisy

It wasn't actually running mergeDeep() and as a perf test, it's a thin composition atop merge() anyhow.
@leebyron leebyron changed the title Adds perf tests for Map.merge and Map.mergeDeep. Adds perf tests for Map.merge. Oct 18, 2017
@leebyron
Copy link
Collaborator

Thanks! I added a few additional changes. I followed up with the change I mentioned before about ensuring the second map contains different values. I also removed mergeDeep() since it seemed broken (it wasn't calling mergeDeep()) and that's just a thin layer atop merge() anyhow, so from a perf testing point of view, it's probably duplicative

@leebyron leebyron merged commit c151990 into immutable-js:master Oct 18, 2017
errendir added a commit to errendir/immutable-js that referenced this pull request Dec 8, 2017
* facebook/master: (305 commits)
  Reduce repetition in COC link `CONTRIBUTING.md`
  Add CODE_OF_CONDUCT.md
  fix typo
  Add link to Code of Conduct in CONTRIBUTING
  Improved Flow support for Record subclasses (immutable-js#1414)
  Improve asImmutable() docs (immutable-js#1415)
  Only include version in require statement in Runkit instances. (immutable-js#1411)
  Add example for OrderedSet subtract. (immutable-js#1410)
  Fix code samples in Immutable.d.ts docs (immutable-js#1408)
  Adds perf tests for `Map.merge`. (immutable-js#1407)
  Move gulpfile into resources/
  Prettier: perf tests
  Prettier: include resources
  Prettify: Use es5 trailing commas
  Merge immutable-js#1403
  Document regression test steps. (immutable-js#1405)
  Demonstrates correct handling of Symbol keys in Map.mergeDeep when the Maps are nested, and initialized with Symbol KV tuples instead of plain JS object literals. (immutable-js#1404)
  Fix Map#concat (immutable-js#1402)
  test(ts-definitions): test Immutable.d.ts validity using tsc (immutable-js#1401)
  4.0.0-rc.9
  ...
errendir added a commit to errendir/immutable-js that referenced this pull request Dec 8, 2017
* facebook/master: (305 commits)
  Reduce repetition in COC link `CONTRIBUTING.md`
  Add CODE_OF_CONDUCT.md
  fix typo
  Add link to Code of Conduct in CONTRIBUTING
  Improved Flow support for Record subclasses (immutable-js#1414)
  Improve asImmutable() docs (immutable-js#1415)
  Only include version in require statement in Runkit instances. (immutable-js#1411)
  Add example for OrderedSet subtract. (immutable-js#1410)
  Fix code samples in Immutable.d.ts docs (immutable-js#1408)
  Adds perf tests for `Map.merge`. (immutable-js#1407)
  Move gulpfile into resources/
  Prettier: perf tests
  Prettier: include resources
  Prettify: Use es5 trailing commas
  Merge immutable-js#1403
  Document regression test steps. (immutable-js#1405)
  Demonstrates correct handling of Symbol keys in Map.mergeDeep when the Maps are nested, and initialized with Symbol KV tuples instead of plain JS object literals. (immutable-js#1404)
  Fix Map#concat (immutable-js#1402)
  test(ts-definitions): test Immutable.d.ts validity using tsc (immutable-js#1401)
  4.0.0-rc.9
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants