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

non-blocking upload API (streaming small chunks over WS) #5126

Merged
merged 1 commit into from
May 14, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented May 13, 2019

Upload via streaming small chunks over the web-socket.

It's breaking to redesign APIs. I've updated CHANGELOG with a record for it.

@akosyakov akosyakov changed the title non-blocking upload API non-blocking upload API (streaming small chunks over WS) May 13, 2019
@akosyakov
Copy link
Member Author

@AlexTugarev @geropl @elaihau @lmcbout could you try this PR instead?

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload_ws branch 2 times, most recently from 51b1630 to 38f4ea6 Compare May 13, 2019 12:08
@lmcbout
Copy link
Contributor

lmcbout commented May 13, 2019

@akosyakov I started to test this PR. I Drag & Drop a 120 Meg file and it went ok
I Drag & Drop the "Theia" project and its completed the transfer.
Some observation:

  • When I selected Theia and Drop it to the workspace folder, the front-end went "OFFline" when it started
  • The browser froze for about ~20 sec, then it started to respond again
  • Transferring Theia onto the workspace took ~ 8 minutes
    This is my initial observation about this PR, I continue testing it.

@akosyakov
Copy link
Member Author

akosyakov commented May 13, 2019

Upload happens in 2 phases indexing files and streaming them. Indexing happens synchronously.

@lmcbout Do you get Offline and 20 sec at the beginning? I could try to break indexing with setImmediate that UI gets a bit more responsive. My machine seems to be fast enough to make blocking during indexing invisible.

Transferring Theia onto the workspace took ~ 8 minutes

Does it feel slow or reasonable? The issue is that upload share web socket connection with other UI operations. More UI operations you do slower it is. I could try to have a temporary dedicated web socket for it. Not sure whether it is going to help. Maybe it is optimizations which we can do with a follow-up PR?

@lmcbout
Copy link
Contributor

lmcbout commented May 13, 2019

@akosyakov I think it the browser was not going offline, the initial settings for indexing would be fine.So setImmediate could solve it.

Transferring Theia to workspace ~ 8 minutes.
I understand there are many files to transfer, but I find it slow even if the browser does not freeze.
[Suggestion] I am wondering, when indexing the files to transfer, if there are many files, would it be possible to compress it?, may be it could improve the transfer time

@akosyakov
Copy link
Member Author

I understand there are many files to transfer, but I find it slow even if the browser does not freeze.

Could you clarify against what you are comparing? What time would you expect?

@akosyakov
Copy link
Member Author

@lmcbout I've reworked it to use a dedicated web socket. Could you please check how it affects upload speed for you?

@lmcbout
Copy link
Contributor

lmcbout commented May 13, 2019

@akosyakov The last commit version (31ae348) does not compile :(

@elaihau
Copy link
Contributor

elaihau commented May 13, 2019

lerna ERR! [compile] src/node/filesystem-backend-contribution.ts(67,17): error TS2304: Cannot find name 'id'.
lerna ERR! [compile] src/node/filesystem-backend-contribution.ts(68,36): error TS2339: Property 'close' does not exist on type 'NodeFileUploadService'.
lerna ERR! [compile] src/node/filesystem-backend-contribution.ts(68,42): error TS2304: Cannot find name 'id'.
lerna ERR! [compile] theiaext compile exited with code 2

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload_ws branch from 31ae348 to 1e0ca14 Compare May 13, 2019 17:54
@akosyakov
Copy link
Member Author

@lmcbout @elaihau sorry, i've pushed again

@lmcbout
Copy link
Contributor

lmcbout commented May 13, 2019

@akosyakov It looks like the last commit ( 1e0ca14 ) is faster in indexing, but still takes time and we can see the "Offfline" on the status bar. What is wrong with this version is the transfer of files, it does not transfer all the files, the progress % is not updated either.
If I take the earlier commit ( 51b1630) , indexing takes longer and the file transfer takes time ( ~ 8 min) but it completes the transfer, so between the two versions, the commit (51b1630) is the only one right now doing all the jobs.
Question: what should be an acceptable time to Drag & Drop Theia project? (I also have a few plugins with it for a total number of files ~ 65000 files)

@akosyakov
Copy link
Member Author

What is wrong with this version is the transfer of files, it does not transfer all the files, the progress % is not updated either.

Also noticed it, i don't handle empty files properly, going to push a fix soon.

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload_ws branch from 1e0ca14 to 1b59186 Compare May 13, 2019 19:33
@akosyakov
Copy link
Member Author

@lmcbout should be good now

@lmcbout
Copy link
Contributor

lmcbout commented May 13, 2019

@akosyakov Leaving in a few minutes, I will test it tomorrow

@akosyakov
Copy link
Member Author

akosyakov commented May 13, 2019

I've tested with uploading Theia repo (without running yarn) against google drive and this PR in Gitpod. In order to upload to google drive it takes 13m 34s. In order to upload to Gitpod it takes 2m 45s. I would say it is enough.

Also there is no such goal as to make upload fast for this PR, but stable with responsive UI during upload. I think it is already achieved, browsers don't crash anymore.

I will try to make indexing less aggressive, e.g. read dirs one after another, not in parallel, in order to make UI more responsive.

@elaihau
Copy link
Contributor

elaihau commented May 13, 2019

I tested the latest change it in gitpod.

  • the browser had been very slow (but still responsive) for ~15 seconds since i drag-and-drop theia folder into my workspace. After that 15 seconds, it became faster and the experience as a user was back to normal.
  • the status bar didn't become yellow.
  • the percentage that indicates the uploading progress looked normal - it went from 0 all the way up to 100 on completion.

in terms of file upload, i would say I am happy with the experience as a user

@akosyakov
Copy link
Member Author

@lmcbout @elaihau i've reworked once again with indexing and uploading happens in parallel, so indexing should not create high pressure on the disk anymore. It makes progress reporting tricky since i cannot compute total size without indexing all files, so i report uploaded and total files in additional to the percentage. Please try again.

@lmcbout
Copy link
Contributor

lmcbout commented May 14, 2019

@akosyakov
I don't see anymore the offline while indexing at the beginning +1 :)
Files transfer (Theia project) is a mid size project, so I think i is normal it takes a few minutes to upload. The first upload seems to take about the same time as before, but with the indication for the number of files out of Total, it gives the user that something is going on while he can still uses the browser +1
One thing I notice, it takes less time than before because I had the java LSP kicking in at the same time.
I also notice the % is off with the numbers of files but it catches up and shows a proper % after.
So transferring about 60K files in about 4 minutes, I consider it good.
If someone else want to try before merging,

Good job @akosyakov

Copy link
Contributor

@elaihau elaihau left a comment

Choose a reason for hiding this comment

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

i also tested the latest change on gitpod. looks good. thank you !

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload_ws branch from 630b6a6 to 3de1582 Compare May 14, 2019 12:58
@AlexTugarev
Copy link
Contributor

It works really nice @akosyakov! 🎉 Thanks for tackling this!

One thing: I would be great to use a temporary filename which is considered as ignored in the navigator as well as for git. Otherwise it reports a lot of changes, as it updates continuously, which leads to a high cpu load.

Screen Shot 2019-05-14 at 15 09 40

@akosyakov
Copy link
Member Author

@AlexTugarev good point, will have a look

@AlexTugarev
Copy link
Contributor

Just for completeness: the high number of onDidFilesChanged events appears to happen if you try to upload a directory with many files.

Signed-off-by: Anton Kosyakov <[email protected]>
@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload_ws branch from 3de1582 to 3b097ac Compare May 14, 2019 13:52
@akosyakov
Copy link
Member Author

@AlexTugarev exclude even reporting for temporary upload files, for git a user will need to add a pattern to gitignore file

good to merge?

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Great work @akosyakov!

@akosyakov akosyakov merged commit 0bd30d5 into master May 14, 2019
@akosyakov akosyakov deleted the ak/non_blocking_file_upload_ws branch May 14, 2019 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants