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

Defer calling canEnableFFmpegOptimizations on module load #9859

Closed
JMTK opened this issue Sep 29, 2023 · 3 comments · Fixed by #9918
Closed

Defer calling canEnableFFmpegOptimizations on module load #9859

JMTK opened this issue Sep 29, 2023 · 3 comments · Fixed by #9918

Comments

@JMTK
Copy link
Contributor

JMTK commented Sep 29, 2023

Which package is this bug report for?

@discord.js/voice

Issue description

On app startup, I noticed that there were calls to canEnableFFmpegOptimizations(), despite not actually doing anything with voice for some testing I was doing. I do include discord.js/voice code elsewhere. It seems like in https://github.com/discordjs/voice/blob/309ac8596cac422cf22e51331869e011c720124c/src/audio/TransformerGraph.ts#L169, it is calling it just when the module is require/imported. I think this could be moved elsewhere somehow to only call it the first time the nodes are actually referenced somewhere, which should improve app startup time.

Code sample

  1. bot.js:
import Discord from 'discord.js';
import * as DiscordVoice from '@discord.js/voice';
  1. Run node --prof bot.js

  2. Run node --prof-process isolate...log > processed.txt:

 [C++ entry points]:
   ticks    cpp   total   name
    138    1.3%    JS: ~spawnSync node:internal/child_process:1109:19
    138  100.0%      JS: ~spawnSync node:child_process:831:19
    138  100.0%        JS: ~getInfo /discordbot/node_modules/prism-media/src/core/FFmpeg.js:123:17
    138  100.0%          JS: ~canEnableFFmpegOptimizations /discordbot/node_modules/@discordjs/voice/dist/index.js:2234:38
    138  100.0%            JS: ~<anonymous> /discordbot/node_modules/@discordjs/voice/dist/index.js:1:1

Versions

  • discord.js: 14.13.0
  • @discordjs/voice: 0.16.0

Issue priority

Low (slightly annoying)

Which partials do you have configured?

Not applicable

Which gateway intents are you subscribing to?

Not applicable

I have tested this issue on a development release

No response

@balogun14
Copy link

@JMTK can you explain this a bit more

@JMTK
Copy link
Contributor Author

JMTK commented Sep 30, 2023

@JMTK can you explain this a bit more

Sure!

I was recently profiling my bot to investigate sporadic CPU spikes. While I was doing this, unrelated to anything I was actually looking for, I noticed that every time I was profiling on bot startup without doing anything, I saw spawnSync > ffmpeg > canEnableFFmpegOptimizations in the call stack of the processed CPU profile from using node.js CPU profiling. Upon further investigation, I found that it happens inside TransformerGraph.ts, right in the root of the file as soon as it is imported that function executes.|

image

(IMO) Ideally someone who just importing the voice module shouldn't have to worry about importing it and having synchronously spawned processes to get metadata about FFmpeg without even using ffmpeg in that moment. It is negatively affecting the performance of starting my bot if I use import statements since they're all done as soon as the module loads rather than lazy loaded which would require code changes away from "best practices" of ES6.

Ultimately this would be moved to somewhere else where it would call it initially if it had never been called before when you're actually trying to do something with ffmpeg.

@balogun14
Copy link

@JMTK i Understand the issue now

@Jiralite Jiralite added the has PR label Nov 2, 2023
@kodiakhq kodiakhq bot closed this as completed in #9918 Nov 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants