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

bug: playing a video from assets is super slow #2074 #2087

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Jun 19, 2020

fix: #2074

Backstory - I uploaded a large 100mb mp4 video as an asset (using postgres as the db) and added an html5 video element to play it in a page. As soon as the page was trying to load, my wiki server just stopped responding for a good few minutes. Cpu was spiking at 100%.
After a lot of investigation, I came to realize that the issue occurs due to a performance bug in the node-postgress library. I already fixed the bug and waiting for CR on it. and it was released and it is already being used in the master branch.

This PR fixes a performance issue while handling videos:
When a request is aborted (due to change of playing position for example) the browser cancels the request, but the error handling in the code ignored it and tried to send the file again from DB which wastes time and causes more issue and the response is already closed.

In addition, I thought about an optimization - instead of fetching assets from db and caching them, the assets might already be cached on the local disk on one of the storage modules (git or disk), so why not use them?
The reason I came up with the optimization is that even after my fix to node-postgres it still takes 40 whole seconds to retrieve the 100mb video file from the DB and it is simply a bad user experience...

server/models/assets.js Outdated Show resolved Hide resolved
@regevbr
Copy link
Contributor Author

regevbr commented Jun 19, 2020

Im using it so it won't collide with the built in Promise, but as you wish 😀

@NGPixel
Copy link
Member

NGPixel commented Jun 19, 2020

Bluebird implements the standard Promise spec so it's completely fine.

@regevbr
Copy link
Contributor Author

regevbr commented Jun 19, 2020

I'm using typescript 99% of the time and the type definitions are a bit different and can cause minor conflicts, that is why I'm used to do what I did. Will fix in 2 hours.
Any other things to change?

@NGPixel NGPixel marked this pull request as draft June 23, 2020 00:15
@regevbr regevbr force-pushed the #2074 branch 2 times, most recently from 6d65105 to 5e8eaa3 Compare July 7, 2020 14:43
@regevbr regevbr requested a review from NGPixel July 7, 2020 14:43
@regevbr regevbr marked this pull request as ready for review July 7, 2020 14:43
@regevbr
Copy link
Contributor Author

regevbr commented Jul 7, 2020

@NGPixel the bug in the postgress library is finally fixed and released. Can you please finish the CR on this? I would love to see it in the upcoming 2.5 release.
Thanks!

@regevbr regevbr changed the title bug: playing a video from assets is super slow #2074 - WIP bug: playing a video from assets is super slow #2074 Jul 7, 2020
@regevbr
Copy link
Contributor Author

regevbr commented Jul 8, 2020

@NGPixel I rebased the branch so it won't mix commits. Githubs builtin merge with master behavior is not smooth in my opinion, so I do it manually using rebase and force push

@regevbr
Copy link
Contributor Author

regevbr commented Jul 12, 2020

@NGPixel I see you already upgraded the pg dependency in the master branch so the stuck node process issue is solved. This PR still fixes a bug (re-fetching a resource after the request was cancelled) and introduces a performance enhancement of serving static files from storage models that use the local disk. Can you please push this PR before the release of 2.5?

@NGPixel NGPixel merged commit b2ff064 into requarks:master Jul 12, 2020
jionggyu pushed a commit to jionggyu/wiki-2.5.302-patch that referenced this pull request Jul 9, 2024
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.

bug: playing a video from assets is super slow
2 participants