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

[Usage Collection] Small performance improvements #91467

Merged

Conversation

afharo
Copy link
Member

@afharo afharo commented Feb 16, 2021

Summary

This PR replaces 2 usages of array.reduce((acc, [key, value]} => ({...acc, [key]: value}), {}) in favour of Object.fromEntries(array.map(([key, value]) => [key, value])). The performance is much better on the second option, while still maintaining the declarative & non-mutating approach: https://measurethat.net/Benchmarks/Show/11657/0/arrayreduce-vs-objectfromentries-arraymap

NOTE: If these changes look good, I can create an issue to follow up on catching similar usages.

Q: Should we create an eslint rule to somehow recommend the latter?
A: We shouldn't. If we add the mutation-based alternatives, there are many ways to approach this, and different browsers report different performance results. Running https://jsben.ch/UJguY in Safari vs. Chrome vs. Firefox shows that Chrome has a boosted .reduce + mutation of the acc value implementation:

image
From left to right: Safari, Chrome, Firefox.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@afharo afharo added performance Feature:Telemetry v7.13.0 release_note:skip Skip the PR/issue when compiling release notes labels Feb 16, 2021
@afharo afharo force-pushed the usage_collection/small-performance-improvement branch from 34fa438 to c8b1def Compare February 16, 2021 13:06
@afharo afharo force-pushed the usage_collection/small-performance-improvement branch from c8b1def to f48a56c Compare February 16, 2021 13:51
@afharo afharo marked this pull request as ready for review February 16, 2021 16:04
@afharo afharo requested a review from a team as a code owner February 16, 2021 16:04
Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

I feel these are micro-optimizations. I'd rather we'd not put time on.

  1. perf tests are rarely representative of real case scenarios
  2. As a web application there are a lot of bottlenecks (network, threads comminucation, etc). Even if we make the loops and other transformation operations literally take 0ms it will not add provide any speed improvements.

As for readabilty they are both equally readable to me so i have no preference.

@afharo
Copy link
Member Author

afharo commented Feb 18, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@afharo afharo merged commit 70d1d9c into elastic:master Feb 18, 2021
@afharo afharo deleted the usage_collection/small-performance-improvement branch February 18, 2021 15:18
@afharo afharo added v7.12.0 and removed v7.13.0 labels Feb 18, 2021
afharo added a commit to afharo/kibana that referenced this pull request Feb 18, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
afharo added a commit that referenced this pull request Feb 18, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry performance release_note:skip Skip the PR/issue when compiling release notes v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants