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.

Sign up for GitHub

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

Jump to bottom

DCOS-51377: Services Table: avoid rerender (except for status icons) #3796

Merged
merged 11 commits into from
Apr 10, 2019
Merged

DCOS-51377: Services Table: avoid rerender (except for status icons) #3796

merged 11 commits into from
Apr 10, 2019

Conversation

DanielMSchmidt
Copy link
Contributor

Testing

  • Open a services page with react dev tools and flash on rerender enabled and see that there are less then before this change
  • Check that loading icons and deploying status bars still display as before

Trade-offs

Dependencies

Screenshots

Before After first commit
Bildschirmfoto 2019-04-08 um 16 07 45 Bildschirmfoto 2019-04-08 um 16 48 55

@nLight
Copy link
Contributor

nLight commented Apr 9, 2019

legit fails

@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/DCOS-51377-second-try branch 2 times, most recently from a03443f to 2c22720 Compare April 9, 2019 13:37
@natmegs
Copy link
Contributor

natmegs commented Apr 9, 2019

This seems to duplicate #3792 but missing all the service status column stuff?

@DanielMSchmidt DanielMSchmidt mentioned this pull request Apr 10, 2019
Closed
@DanielMSchmidt
Copy link
Contributor Author

DanielMSchmidt commented Apr 10, 2019 •
edited
Loading

@natmegs Because of the accurate service merges there are quite some conflicts for the status column with every PR we merge on that topic. Let's revisit this soon and do a final rebase once the accurate service work is all done 👍

This PR aims to get a good chunk of the progress merged soon.

@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/DCOS-51377-second-try branch from 2c22720 to 93208d2 Compare April 10, 2019 08:45
pierrebeitz
pierrebeitz previously approved these changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@pierrebeitz pierrebeitz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

i opened master and the new branch side by side and behaviour seems to be the same against soak.

on my machine it's rather a ~30% boost instead of the advertised ~50% on scripting.

that's not to disparage these awesome changes! 🎉

EDIT: i looked at the memory consumption and for a single measurement, these changes appear to have increased base consumption while flattening deviation. not tested thoroughly, but i thought this might be worth sharing nonetheless:

OS Enterprise 2019-04-10 11-56-17

(40s each - interestingly you can count 20 bumps on master, while it's hard to see that on this branch)

plugins/services/src/js/containers/services/__tests__/ServicesTableRenderers-test.js Outdated Show resolved Hide resolved
plugins/services/src/js/columns/ServicesTableInstancesColumn.tsx Show resolved Hide resolved
plugins/services/src/js/columns/ServicesTableDiskColumn.tsx Outdated Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/DCOS-51377-second-try branch from 93208d2 to eeb407d Compare April 10, 2019 11:33
@DanielMSchmidt
Copy link
Contributor Author

I split up the commits to be per column for easier rebasing and I also removed the unit tests, because they did not seem to add any value

Daniel Schmidt added 10 commits April 10, 2019 14:42
chore(services): remove tests for columns
e8ea54a 
These tests only test if the business logic values from the structs
are passed correctly to the components, which is a matter of our
integration tests.
perf(services): avoid rerenders for progress bars
2746426 
Closes DCOS-51377
perf(services): avoid rerenders for cpu column
19fd32e 
Closes DCOS-51377
perf(services): avoid rerenders for disk column
75b42df 
Closes DCOS-51377
perf(services): avoid rerenders for gpu column
708fdea 
Closes DCOS-51377
perf(services): avoid rerenders for instances column
f1dd27c 
Closes DCOS-51377
perf(services): avoid rerenders for mem column
47c0ad3 
Closes DCOS-51377
perf(services): avoid rerenders for name column
24e7078 
Closes DCOS-51377
perf(services): avoid rerenders for version column
60524c4 
Closes DCOS-51377
perf(services): avoid rerenders for region column
702400e 
Closes DCOS-51377
@DanielMSchmidt DanielMSchmidt force-pushed the danielmschmidt/DCOS-51377-second-try branch from eeb407d to 702400e Compare April 10, 2019 12:43
juliangieseke
juliangieseke reviewed Apr 10, 2019
View reviewed changes
Copy link
Contributor

@juliangieseke juliangieseke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

some nits one nit, nothing that has to be changed in this PR. one question about tests tho.

plugins/services/src/js/containers/services/__tests__/ServicesTableRenderers-test.js
@@ -1,190 +0,0 @@
import { NumberCell } from "@dcos/ui-kit";
Copy link
Contributor

Choose a reason for hiding this comment

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

did you ensure we have integration tests testing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wrote some of them back then :)

plugins/services/src/js/columns/ServicesTableInstancesColumn.tsx Show resolved Hide resolved
plugins/services/src/js/columns/ServicesTableNameColumn.tsx
</span>
</div>
</TextCell>
<ServiceName
Copy link
Contributor

Choose a reason for hiding this comment

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

'nit: I think it would have been possible to split this into ServiceName and GroupName component 🤔 not sure if I'm missing something and I think its not that important tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as before, I did not touch anything in this direction. I don't see that this is part of the scope of this ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least if our goal is still to merge this functionality today. If this goal changed we can also close this PR and use the #3792 one

juliangieseke
juliangieseke approved these changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@juliangieseke juliangieseke left a comment

Choose a reason for hiding this comment

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

no, lets get it in, we can improve later

@GeorgiSTodorov GeorgiSTodorov merged commit 7d12aa8 into master Apr 10, 2019
@GeorgiSTodorov GeorgiSTodorov deleted the danielmschmidt/DCOS-51377-second-try branch April 10, 2019 14:43
@mesosphere-ci
Copy link
Collaborator

🎉 This PR is included in version 2.81.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

@GeorgiSTodorov GeorgiSTodorov GeorgiSTodorov approved these changes

@juliangieseke juliangieseke juliangieseke approved these changes

@pierrebeitz pierrebeitz pierrebeitz approved these changes

@natmegs natmegs Awaiting requested review from natmegs

@mperrotti mperrotti Awaiting requested review from mperrotti

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@DanielMSchmidt @nLight @natmegs @mesosphere-ci @pierrebeitz @juliangieseke @GeorgiSTodorov