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

Resolve perf issues on the search page #179

Merged
merged 1 commit into from
Feb 6, 2018
Merged

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Feb 4, 2018

Fix #178.

Main issue was two too many stateless functional components, now React.PureComponents.

Also:

  • No longer mutating the array of search results when sorting it
  • Added a couple of missing util css imports

@ghost ghost assigned tiffon Feb 4, 2018
@ghost ghost added the review label Feb 4, 2018
@codecov
Copy link

codecov bot commented Feb 4, 2018

Codecov Report

Merging #179 into master will increase coverage by 0.03%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   90.77%   90.81%   +0.03%     
==========================================
  Files          93       93              
  Lines        1930     1937       +7     
  Branches      383      383              
==========================================
+ Hits         1752     1759       +7     
  Misses        155      155              
  Partials       23       23
Impacted Files Coverage Δ
src/index.js 0% <0%> (ø) ⬆️
src/components/SearchTracePage/index.js 88.37% <100%> (+1.19%) ⬆️
...onents/SearchTracePage/SearchResults/ResultItem.js 100% <100%> (ø) ⬆️
.../components/SearchTracePage/SearchResults/index.js 92.3% <87.5%> (ø) ⬆️
...neViewer/TimelineHeaderRow/TimelineViewingLayer.js 90.74% <0%> (+3.7%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7a3e74...139ab07. Read the comment docs.

@black-adder
Copy link
Contributor

just curious, are there automated ways to detect regressions like this in the UI?

@tiffon
Copy link
Member Author

tiffon commented Feb 5, 2018

@black-adder jaegertracing/jaeger-client-node/benchmarks is one approach. Another is testing through a browser with something like Puppeteer for Chrome — similar to e2e testing but you'd want to capture a perf trace via the dev tools. The former is granular while the latter has sweeping coverage but variance is likely to be a problem.

What's the approach taken with the various Jaeger benchmarks to have some sort of historical perspective or expectation relative to the current benchmark results? E.g. the current perf is X, the new perf is Y.

@saminzadeh
Copy link
Member

Main issue was two too many stateless functional components, now React.PureComponents.

To clarify, the issue is not necessarily stateless components but rather unnecessary render comparisons right? The PureComponent will add the shouldComponentUpdate lifecycle method to do a shallow comparison of props and state to avoid unnecessary renders.

Another alternative would be to keep the components stateless and then use a simple utility like recompose's pure or onlyUpdateForKeys HOCs that would only result in a few lines of code change.

The result would be a smaller code change and easier to remove when we optimize further up the state tree:

export function ResultItem(props) { /*... */ }

export default pure(ResultItem);

@tiffon
Copy link
Member Author

tiffon commented Feb 6, 2018

Another alternative would be to keep the components stateless and then use a simple utility like recompose's pure or onlyUpdateForKeys HOCs that would only result in a few lines of code change.

@saminzadeh That's a great point. My inclination is to use React.PureComponent, but it's a tough call...

@tiffon tiffon merged commit 7dcf75b into master Feb 6, 2018
@ghost ghost removed the review label Feb 6, 2018
@yurishkuro yurishkuro deleted the issue-178-search-page-perf branch January 29, 2020 15:06
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants