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

perf(Role): Use iterable and exit early if found #9490

Closed
wants to merge 5 commits into from
Closed

perf(Role): Use iterable and exit early if found #9490

wants to merge 5 commits into from

Conversation

JMTK
Copy link
Contributor

@JMTK JMTK commented May 1, 2023

Please describe the changes this PR makes and why it should be merged:
This improves performance by not copying the role sorted collection into an array, which is how the spread syntax treats it. This should help with both memory and CPU since it creates a copy of the array. This does not fully address #3925 but would help mitigate some performance issues related to access the position property frequently.

Some profiling:

23000 iterations
Old: 66.575ms
New: 30.693ms
1 million iterations
Old: 761.495ms
New: 344.201ms
10 million iterations
Old: 7.964s
New: 3.588s

Status and versioning classification:
Patch

@JMTK JMTK requested a review from a team as a code owner May 1, 2023 19:51
@JMTK JMTK requested review from vladfrangu, iCrawl, kyranet and SpaceEEC May 1, 2023 19:51
@vercel
Copy link

vercel bot commented May 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) May 1, 2023 8:08pm
discord-js-guide ⬜️ Ignored (Inspect) May 1, 2023 8:08pm

@Jiralite Jiralite changed the title Use iterable and end early if found perf(Role): Use iterable and exit early if found May 1, 2023
@Jiralite Jiralite added this to the discord.js 14.11 milestone May 1, 2023
Thanks! Good catch

Co-authored-by: Aura Román <kyradiscord@gmail.com>
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

@JMTK JMTK requested a review from kyranet May 1, 2023 20:07
@almeidx
Copy link
Member

almeidx commented May 1, 2023

Couldn't this also be applied to GuildChannel#position? Or is it not relevant

get position() {
const sorted = this.guild._sortedChannels(this);
return [...sorted.values()].indexOf(sorted.get(this.id));
}

@JMTK
Copy link
Contributor Author

JMTK commented May 1, 2023

Couldn't this also be applied to GuildChannel#position? Or is it not relevant

Yeah definitely could. In my own use case I had a similar issue as the open issue on the repo, but it could definitely have the same type of problems if someone was looping through the channels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants