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

[BREAKING] Convert RoomState's stored state map to a real map #1419

Merged
merged 2 commits into from
Jul 16, 2020

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jul 9, 2020

Requires matrix-org/matrix-react-sdk#4936

We probably do not want to land this until after an RC as it is very high risk.

Though the dictionary format works fine, it's slow. Access times are around the 1ms range for rooms like HQ, which doesn't seem like much, but when compared to the Map's access time of 0.05ms it's slow.

Converting things to a map means we lose index access and have to instead use .keys() and .values(), both of which return iterables and not arrays. Even doing the Array.from() conversion we see times in the 0.05ms range. This is also what makes this a breaking change.

Memory-wise there does not appear to be any measurable impact for a large account.

Though the dictionary format works fine, it's slow. Access times are around the 1ms range for rooms like HQ, which doesn't seem like much, but when compared to the Map's access time of 0.05ms it's slow.

Converting things to a map means we lose index access and have to instead use `.keys()` and `.values()`, both of which return iterables and not arrays. Even doing the `Array.from()` conversion we see times in the 0.05ms range. This is also what makes this a breaking change.

Memory-wise there does not appear to be any measurable impact for a large account.
@turt2live
Copy link
Member Author

As an interesting bit of testing: Object.values is relatively O(n) with its call time with fluctuations of +/-0.4ms on the exact same data set. Our very own utils.values is also O(n) but more stable at a +/-0.05ms variation with the same data set. Our implementation is also consistently faster at ~0.87ms for a 5.1k data set whereas Object.values takes ~1.1ms

@jryans
Copy link
Collaborator

jryans commented Jul 9, 2020

Our very own utils.values is also O(n) but more stable at a +/-0.05ms variation with the same data set.

That's quite surprising...! 😱 Definitely would have expected Object.values to be optimised by JS engines more than a manual version, but perhaps they do some checks we are able to assume and skip.

@jryans
Copy link
Collaborator

jryans commented Jul 9, 2020

@turt2live Which part is so high risk about this...? The late-breaking change to APIs?

@turt2live
Copy link
Member Author

It's high risk because I can't guarantee that I've caught all the places we use the public property. A combination of being 2016-era JS and generically named means there's huge potential that a reference was missed.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

The performance delta is insane!

src/models/room-state.js Outdated Show resolved Hide resolved
@turt2live turt2live merged commit 7d398d4 into develop Jul 16, 2020
@turt2live turt2live deleted the travis/general/perf/js-roomstate-map branch July 16, 2020 18:42
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