-
Notifications
You must be signed in to change notification settings - Fork 889
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
Disable http disk cache and implempent in-memory image cache #2498
Conversation
_scripts/dev-runner.js
Outdated
electronProcess = spawn(electron, [ | ||
path.join(__dirname, '../dist/main.js'), | ||
// '--enable-logging', Enable to show logs from all electron processes | ||
// '--enable-logging', // Enable to show logs from all electron processes | ||
remoteDebugging ? '--inspect=9222' : '', | ||
remoteDebugging ? '--remote-debugging-port=9223' : '', | ||
]) | ||
], | ||
// { stdio: 'inherit' } // required for logs to actually appear in the stdout | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ stdio: 'inherit' }
makes spawn()
pipe the stdout and stderr of the spawned command to the stdout and stderr of the parent process, in this case _scripts/dev-runner.js
. Without that change you won't see any logs from the main
process, I've left it in there commented out so that if someone needs to log something from the main
process in the future they don't need to spend hours trying to work out why they can't see the logs, like I did.
Adding the extra //
to the enable logging line, makes it easier to use your IDE's keyboard shortcuts to uncomment the line without needing to go and add the slashes in manually after you have uncommented it. As Enable to show logs from all electron processes
is not valid javascript code and will need to stay commented even when you uncomment that line.
src/main/index.js
Outdated
@@ -48,6 +50,10 @@ function runApp() { | |||
app.commandLine.appendSwitch('enable-file-cookies') | |||
app.commandLine.appendSwitch('ignore-gpu-blacklist') | |||
|
|||
// the http cache causes excessive disk usage during video playback | |||
// we've got a custom image cache to make up for disabling the http cache | |||
app.commandLine.appendSwitch('disable-http-cache') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has --disk-cache-size=0
been tried?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep and it doesn't make a difference to disk usage during video playback. I also tried setting it to 1 which makes it only do 0.1MB/s which is already a lot better than it was, but it doesn't eliminate the problem completely. I suspect that setting it to 0 tells it to just use their default limit.
Using it locally (MacOS) |
I've added fallback so for the expiry timestamp so that it should never fail, if none of the headers are available it defaults to a hardcoded 2 hour expiry time (can be changed with the As discussed I've also moved these changes behind a CLI argument so that this behaviour is opt in for now. (If you want to enable it while using |
I wonder how this can be enabled when released... |
On Windows you can either run the .exe from the CLI e.g. On Linux you can either run FreeTube from the CLI or add it to the macOS is annoying though, you can only pass the flag when you launch FreeTube from the terminal: |
Really need a new settings to enable/disable this... (and be effective on restart) |
Finally figured out a way to add a GUI setting for this. Also removed the CLI flag. |
copy of @PikachuEXE's comment here #2511 (comment)
|
I still see error popups on windows, disabled on windows for now |
This can almost be merged except those random JS errors |
Also just found out the code is outdated due to settings panel update |
src/main/index.js
Outdated
callback({ | ||
statusCode: response.statusCode ?? 400 | ||
}) | ||
throw error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PikachuEXE I suspect the error you are seeing is happening here. Instead of throwing it I could probably return the error in the response and then it would be visible in the devtools instead of throwing up a random error popup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it behaves the same as when it's disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so instead of throwing the error it will return it as JSON, so you should be able to see it in the network tab in the devtools. I still don't understand why the error is happening in the first place, but this should at least make errors less intrusive and should make it easier for you to debug it. Of course the images that caused the errors still won't load but you shouldn't get the popup now.
Please let me know when you have some error JSON so I can try and work out what the problem is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK to merge as an experimental setting
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
61ba38e
to
4d3c1d4
Compare
Disable http disk cache and implempent in-memory image cache
Important note
We may remove your pull request if you do not use this provided PR template correctly.
Pull Request Type
Related issue
#1837 could be related but I'm not sure
Description
Thank you to the Matrix user for bringing this issue to our attention.
To reduce RAM usage electron stores the http cache on disk. While that might be alright for other applications, in FreeTube that means that while you are watching a video, all of the buffered video data is actually being constantly streamed to and from your disk. In the long term this will likely have an inpact on disk health.
This pull request is made up of two parts:
How the cache works:
When FreeTube tries to load an image over HTTP(S), the request gets redirected to the
imagecache://
custom protocol handler. The protocol handler checks the cache for the image, if it finds it, the image data is returned. Otherwise it fetches the image, stores it to the cache and then returns the image data.The amount of time an image stays in the cache is based on the
Cache-Control: max-age=<seconds>
andAge: <seconds>
headers that are returned by the server when the image is fetched. The cache checks for and removes expired images every 5 minutes, this interval can altered in the future if needed.Performance:
In my testing disabling the http cache reduced the disk usage while playing a 1080p DASH video from 1.4 - 2.2MB/s to 0MB/s, when you first start the video it will sit at about 0.1MB/s as the DASH manifest and storyboards are written to the disk and subsequently used.
I tried profiling the memory usage of the image cache using the
--inspect-brk
flag and connecting to the main process using the devtools in an external chromium browser. Unfortunately I'm not quite sure what the actual memory usage is, as the heap snapshot said it was using about 28.4MB for 1053 images, however the entire heap was only 19.1MB at the time of the snapshot, so something isn't adding up.Screenshots (if appropriate)
Please add before and after screenshots if there is a visible change.
Testing (for code that is not small enough to be easily understandable)
Please test this PR with production/release builds as dev and debug builds use extra RAM because of the devtools.
To check the disk usage I used the Task Manager in Windows to monitor the disk usage during video playback (e.g. https://youtu.be/bafzQBSktwk)
RAM usage can be monitored using the Task Manager or by hooking up the devtools in an external chromium installation up to the main process:
--inspect-brk
(allows remote debugging of the main process and wait until you are ready before executing any javascript code).chrome://inspect
ImageCache
won't be calledImageCache
, instead it will have a single letter name, in my case it wasq
. You can find the name by searching forNo image cache entry for
in the Sources tab, going back a bit until you find theconstructor
and then looking at the class name.Desktop (please complete the following information):
Additional context
If the uptick in RAM usage is too significant we can always consider caching the images on disk instead of in-memory. This will be slower but will have the advantage of lower RAM usage. It will also allow us to use the cached images, that haven't expired yet, the next time you open FreeTube.