Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Improve permalink performance #2671

Merged
merged 23 commits into from
Feb 26, 2019
Merged

Improve permalink performance #2671

merged 23 commits into from
Feb 26, 2019

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Feb 21, 2019

While profiling for element-hq/element-web#8565, I noticed creating event permalinks in EventTile::render took 25ms in a room of 6k members. Paginating in 20 event tiles freezes the UI for 1.2 seconds in this case, so I thought it made sense to solve this first as any other improvements in this area would be unnoticeable.

The solution is to make the permalink code track changes in the room and update the server candidates only when needed.

@bwindels bwindels changed the title Improve perma link performance Improve permalink performance Feb 21, 2019
@bwindels bwindels removed the notready label Feb 21, 2019
@bwindels bwindels requested a review from a team February 21, 2019 17:20
@turt2live turt2live self-assigned this Feb 21, 2019
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Overall structure looks sane, just a few comments regarding the algorithm

@@ -525,6 +525,7 @@ module.exports = React.createClass({
eventSendStatus={mxEv.status}
tileShape={this.props.tileShape}
isTwelveHour={this.props.isTwelveHour}
permaLinkCreator={this.props.permaLinkCreator}
Copy link
Member

Choose a reason for hiding this comment

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

extremely non-consequential nit: permalink is one word

src/matrix-to.js Outdated

_updateHighestPlUser() {
const plEvent = this._room.currentState.getStateEvents("m.room.power_levels", "");
const content = plEvent.getContent();
Copy link
Member

Choose a reason for hiding this comment

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

something I ran into with Settings is that the power levels event might not exist. A null check here would prevent React from exploding

src/matrix-to.js Outdated
}, [null, 0]);
const [userId, powerLevel] = maxEntry;
// object wasn't empty, and max entry wasn't a demotion from the default
if (userId !== null && powerLevel > (content.users_default || 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

should be >= 50 - we intentionally don't check if they have > state_default and definitely don't want people who are low in the power level department.

(the comment block at the top explains why)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, my bad

src/matrix-to.js Show resolved Hide resolved
src/matrix-to.js Show resolved Hide resolved
const parser = document.createElement('a');
parser.href = "https://" + domain;
return parser.hostname;
return new URL(`https://${domain}`).hostname;
Copy link
Member

Choose a reason for hiding this comment

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

There was a reason why this wasn't using the URL constructor, but I can't remember why :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's well supported fwiw ... https://caniuse.com/#search=url Node also has it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to put it back in if you think it will break stuff though.

Copy link
Member

Choose a reason for hiding this comment

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

It had something to do with how it parsed stuff and not working in electron, but evidently all those concerns don't exist anymore. It should be fine like this.

test/matrix-to-test.js Show resolved Hide resolved
@turt2live turt2live assigned bwindels and unassigned turt2live Feb 21, 2019
@bwindels bwindels assigned turt2live and unassigned bwindels Feb 22, 2019
@bwindels
Copy link
Contributor Author

Thanks for the review! Should be ready for another look.

src/matrix-to.js Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

turt2live commented Feb 22, 2019

(also in future please hit the "request review" button because I have a tenancy to lose these sorts of things in the depths of my notifications)

@turt2live turt2live assigned bwindels and unassigned turt2live Feb 22, 2019
@bwindels
Copy link
Contributor Author

(also in future please hit the "request review" button because I have a tenancy to lose these sorts of things in the depths of my notifications)

Arhhg, I keep forgetting this, sorry! Will try to be mindful of it.

@bwindels bwindels force-pushed the bwindels/permalinkperf branch 2 times, most recently from a24b973 to 137bda4 Compare February 25, 2019 15:51
@bwindels bwindels requested a review from turt2live February 25, 2019 17:27
@bwindels bwindels force-pushed the bwindels/permalinkperf branch from 137bda4 to 77f979e Compare February 25, 2019 17:43
@bwindels bwindels merged commit 68ba149 into develop Feb 26, 2019
jryans added a commit to jryans/matrix-react-sdk that referenced this pull request Mar 11, 2019
After the permalink API was changed in
matrix-org#2671, it seems we forgot to
update this call site, so it was creating `<room>/<room>` links, instead of
`<room>/<event>`.

Fixes element-hq/element-web#9110
krombel pushed a commit to krombel/matrix-react-sdk that referenced this pull request Mar 15, 2019
After the permalink API was changed in
matrix-org#2671, it seems we forgot to
update this call site, so it was creating `<room>/<room>` links, instead of
`<room>/<event>`.

Fixes element-hq/element-web#9110
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants