tjibbevanderlaan / chromeos-filesystem-sftp

ChromeOS app to access SFTP server
https://chrome.google.com/webstore/detail/shared-network-folder-sft/gbheifiifcfekkamhepkeogobihicgmn
BSD 3-Clause "New" or "Revised" License
80 stars 21 forks source link

Slow performance with large files; using Base64 #55

Closed fedoracooper closed 9 years ago

fedoracooper commented 9 years ago

First of all, excellent job with this app! This is a very valuable tool.

I am getting very poor read performance on my LAN via this SFTP plugin. Getting a large directory listing is pretty slow, but downloading large files really makes it obvious. I am getting about 5Mbit / sec on my LAN over wireless N, on an Asus C300 chromebook. I am connecting to a Linux machine running Fedora 20. When I shared the same files via HTTPS, I was able to download at more than 10 times the speed, around 60-70 Mbit / sec. Something is not right.

I spent some time digging into the code. This stuff is all new to me, but after some reading, it looks like one possible reason is the use of JSON for passing of all data between NaCl and Javascript. The use of JSON has apparently forced you to Base64 encode and decode all the data. Thus, for every chunk of data read from the network, we are encoding into Base64, wrapping in JSON, sending to Javascript, which then decodes the Base64 and finally sends the binary data to the File System API.

Now maybe this works okay on high powered machines with tons of CPU, but this Asus is not the fastest of the chromebooks. CPU usage is definitely high while transferring files. I did some research into passing of data via Pepper, and found that Pepper clearly supports binary data via the pp::VarArrayBuffer datatype. I suppose that JSON is being used because it can handle multiple values (request ID, data, etc.). However, it looks like the pp::Var datatype used by Pepper supports multiple values in a key -> value format using pp::VarDictionary.

So in summary, I suspect Base64 / JSON is the cause of the performance issues, and it should be replaced by using VarDictionary containing strings and a VarArrayBuffer to avoid unnecessary translation.

Of more minor concern is the buffer size when pulling data from SFTP; it looks like 2KB is being used as the buffer / chunk size, which might not be very efficient due to the typical TCP/IP packet size of 1.5KB. SFTP supports up to 32KB, and I suspect 8KB to 16KB would improve performance.

fedoracooper commented 9 years ago

I updated my original message to reference pp::VarDictionary instead of VaryArray. VarDictionary seems to be a very close replacement of Json::Value.

If I have time, I may be able to test my theory this weekend. If I am successful, I will create a fork and commit my changes.

yoichiro commented 9 years ago

@fedoracooper Yes, you are right. I have been starting the modification against this issue: #56

yoichiro commented 9 years ago

@fedoracooper I have just released the new version 1.6.0 including the modification against this issue.

yoichiro commented 9 years ago

@fedoracooper I close this issue, because I could not find any problems after releasing. Thank you for the reporting!