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

refactor: Move getNode/canEnableFFmpegOptimizations into a lazy loaded call #9918

Merged
merged 8 commits into from
Nov 6, 2023

Conversation

JMTK
Copy link
Contributor

@JMTK JMTK commented Nov 2, 2023

Please describe the changes this PR makes and why it should be merged:
Lazy calls the functions in the root of TransformerGraph.ts, which are called the first time the file is imported(which is through any time @discordjs/voice is imported. More importantly, the call to canEnableFFmpegOptimizations, which is a loop that synchronously spawns child processes is now deferred until the first time you actually attempt to play audio.

This improves bot startup time by 100ms for people importing @discordjs/voice:
Before:
image
After:
image

I tested this locally and can confirm audio plays correctly and the call is only called then.

Resolves #9859

Status and versioning classification:

  • Patch

… the first time it's actually ever referenced
@JMTK JMTK requested a review from a team as a code owner November 2, 2023 21:18
Copy link

vercel bot commented Nov 2, 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) Visit Preview Nov 6, 2023 9:15am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2023 9:15am

Copy link

github-actions bot commented Nov 2, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 66
🟢 Accessibility 96
🟢 Best practices 100
🟠 SEO 67
🔴 PWA 33

Lighthouse ran on https://discord-js-git-fork-jmtk-main-discordjs.vercel.app/

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #9918 (4c7a95d) into main (ac64508) will decrease coverage by 0.18%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main    #9918      +/-   ##
==========================================
- Coverage   60.22%   60.05%   -0.18%     
==========================================
  Files         235      235              
  Lines       16114    16028      -86     
  Branches     1231     1216      -15     
==========================================
- Hits         9705     9625      -80     
+ Misses       6365     6359       -6     
  Partials       44       44              
Flag Coverage Δ
voice 63.64% <57.14%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/voice/src/audio/TransformerGraph.ts 75.38% <57.14%> (+1.19%) ⬆️

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@Jiralite Jiralite added this to the voice 0.17.0 milestone Nov 2, 2023
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

👍

@Jiralite Jiralite changed the title Move getNode/canEnableFFmpegOptimizations into a lazy loaded call refactor: Move getNode/canEnableFFmpegOptimizations into a lazy loaded call Nov 4, 2023
@Jiralite Jiralite removed the blocked label Nov 4, 2023
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.

Alternative idea, what if we make NODES nullable and initialize using nullish assignment?

Similar to:

const node = (NODES ??= initializeNodes()).get(type);

(Or splitting the expression in two lines). Obviously this change would make initializeNodes() return a newly constructed map.

The reasoning behind this idea is so NODES, being null rather than an empty map guards against the possible edge case of using an unitialized map elsewhere.

It would also remove the need for an extra variable and extra operations.

packages/voice/src/audio/TransformerGraph.ts Outdated Show resolved Hide resolved
…ODES, this removes an extra variable and cleans up the code
kyranet
kyranet previously requested changes Nov 5, 2023
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.

Needed for the ESLint check to pass:

packages/voice/src/audio/TransformerGraph.ts Outdated Show resolved Hide resolved
Co-authored-by: Aura Román <kyradiscord@gmail.com>
@Jiralite
Copy link
Member

Jiralite commented Nov 6, 2023

Please run Prettier!

@JMTK
Copy link
Contributor Author

JMTK commented Nov 6, 2023

Please run Prettier!

Done! Didn't realize that space was still there

@kodiakhq kodiakhq bot merged commit 637e1a4 into discordjs:main Nov 6, 2023
21 checks passed
cyan-2048 pushed a commit to cyan-2048/discord.js that referenced this pull request May 8, 2024
…oaded call (discordjs#9918)

* Move the getNode/canEnableFFMPEGOptimizations into a lazy loaded call the first time it's actually ever referenced

* PR feedback: Make initializeNodes return a map then nullably assign NODES, this removes an extra variable and cleans up the code

* chore: lint suggestion

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

* Use local map instead of recursive

* Run prettier

* Fix lint

---------

Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: Aura Román <kyradiscord@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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.

Defer calling canEnableFFmpegOptimizations on module load
5 participants