Desktop: Speed up Webdav Sync on Linux #2577
Conversation
|
The code looks good, but please install the pre-commit hook. Your code breaks Travis due to linter errors: 24:24 error Trailing spaces not allowed no-trailing-spaces
25:48 error Strings must use singlequote quotes
25:100 error Strings must use singlequote quotes
30:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs
31:5 error Expected space(s) before "else" keyword-spacing
31:5 error Expected space(s) after "else" keyword-spacing
36:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs
37:1 error Expected indentation of 3 tabs but found 24 spaces indent |
|
Btw, I will test this on macOS as well. Keep alive is not a bad idea anyway. Perf on desktop is worse compared to mobile, because http/1.1 is used instead of h2. |
ReactNativeClient/lib/WebDavApi.js
Outdated
| @@ -5,6 +5,10 @@ const JoplinError = require('lib/JoplinError'); | |||
| const URL = require('url-parse'); | |||
| const { rtrimSlashes } = require('lib/path-utils.js'); | |||
| const base64 = require('base-64'); | |||
| const Setting = require('lib/models/Setting.js'); | |||
ReactNativeClient/lib/WebDavApi.js
Outdated
| @@ -17,6 +21,21 @@ class WebDavApi { | |||
| this.logger_ = new Logger(); | |||
| this.options_ = options; | |||
| this.lastRequests_ = []; | |||
| if (shim.isLinux()) { | |||
| if (Setting.value('sync.6.path').startsWith("https") || Setting.value('sync.5.path').startsWith("https")) { | |||
There was a problem hiding this comment.
No Setting here please. This is a generic class that shouldn't have back references.
ReactNativeClient/lib/WebDavApi.js
Outdated
| keepAlive: true, | ||
| maxSockets: 1, | ||
| keepAliveMsecs: 5000, | ||
| }); | ||
| }else{ | ||
| this.webDavAgent_ = new http.Agent({ | ||
| keepAlive: true, | ||
| maxSockets: 1, | ||
| keepAliveMsecs: 5000, |
There was a problem hiding this comment.
Is there any similarity between the agents? Can't this be simplified?
ReactNativeClient/lib/WebDavApi.js
Outdated
| @@ -31,6 +50,7 @@ class WebDavApi { | |||
| options.headers = Object.assign({}, options.headers); | |||
| if (options.headers['Authorization']) options.headers['Authorization'] = '********'; | |||
| delete options.method; | |||
| if (shim.isLinux()) delete options.agent; | |||
ReactNativeClient/lib/WebDavApi.js
Outdated
| @@ -357,6 +377,7 @@ class WebDavApi { | |||
| const fetchOptions = {}; | |||
| fetchOptions.headers = headers; | |||
| fetchOptions.method = method; | |||
| if (shim.isLinux()) fetchOptions.agent = this.webDavAgent_; | |||
|
|
Thanks for pointing those out, I think this commit should be more elegant now. Anything left? |
|
Thanks for the update. Does the app build and sync WebDAV on mobile with this change? If not, the logic needs to be abstracted away using the shim. |
|
I've tested this on macOS and it makes no difference. With it it takes 9 seconds for a sync operation with no changes, and without it, it also takes 9 seconds. Something is really weird. In Joplin versions from half a year ago, it took about 2 seconds. |
|
Sorry, I don't have the enviroment to build the android app right now, but why exactly shouldn't it work? The only thing I see outside the linux only block is a delete command and the loading of the http and https package, both are part of the internal NodeJS library Edit: |
|
I'm actually getting 2 errors: So it seems these 2 (http, https) can only be loaded when not on mobile. Not sure why the first one happens. |
|
FYI: It's a very bad idea to not use a local branch. Using the master branch for development can lead to many problems. Just an advice for the future. |
Running
React Native doesn't include Node.js and only emulates some of its modules, so indeed we need to use the shim to handle this. |
|
Thanks for your effort, I included the shim around the modules, so this should hopefully be fixed now. |
|
That won't work unfortunately because Babel is going to include everything. If this PR is too complex we might have to close it again, as I don't want it to become a time sink again. |
|
My experience with React Apps is limited, but is there maybe there is way to ignore this work in the mobile clients? Maybe like described here or by importing it from an external library which is only requested in Linux? |
|
We solve cross-platform issues using the shim (in lib/shim.js). For example, you would create a method on shim-init-node.js (used for desktop and CLI), you override that method with your current code. I'd suggest to lazily create the agent (so create the agent the first time, and save the result to a variable, which you return on subsequent calls). on shim-init-react.js (used for mobile), you make the method return Then in WebDavApi, you simply do |
|
Thank you for your patience |
|
Alright, this version is still functional with electron, but entirely running with the shim setup. |
| @@ -363,6 +365,24 @@ function shimInit() { | |||
| return bridge().openExternal(url); | |||
| }; | |||
|
|
|||
| shim.agent = null; | |||
There was a problem hiding this comment.
shim.httpAgent_ please (that's the convention we use for private variables)
|
Thanks for the update @WisdomCode, just one last thing about the variable name and we're good to merge. |
|
Its corrected, and again tested, so I think it's good to go! |
|
Ok, all good then, thanks @WisdomCode and @tessus! |
|
It is still very slow on Ubuntu 18.04. I'm using version 1.0.201 |
|
I'm experiencing this issue as well. I'm on a fresh install of ElementaryOS 5.1.4 and it took about 8 minutes for it to initially sync all my encrypted notes (3 notebooks, one of which is the default one, and the other two contain <5 notes each). The same documents took <1 minute to sync on my Android phone from scratch. It's not unusable, but it does seem unusually slow and as someone who switches devices often it'll make Joplin trickier to use on my Linux machine. What else could be causing this? |
|
On Linux Mint 19.3 I also experience slowness. I think I noticed something strange. |
|
I’m using Joplin with a Sync over WebDAV to Nextcloud, and -in the past- I’ve also experienced extremely slow syncs on the desktop app, while the mobile app (iOS) was perfectly fine. A few days ago I’ve updated to Nextcloud v25, and the problems all of sudden diminished completely. |
|
@je-s the nc webdav perf always had an issue. it's good to hear that they finally improved it. it's just the core of nextcloud, but it took them more than 5 years to fix the most basic and fundamental issue. Re perf with standard webdav: the problem was never that big, but tha fact that the connection was closed for every request was certainly not helpful and this PR fixed that. Another big perf improvement would be h2, but unfortunately the desktop apps use npm modules that do not speak h2. At least not yet afaik. |
In its basics, this is a second attempt on #1931
I shamelessly took the changes done by @e-ihrke and followed through the problems presented by @laurent22 and @tessus to finish up this older problem presented in its current form best in issue #1023
This keepalive fix sped up my setup enormously, it is now comparable to my android app. Before, it took about 5 minutes to sync a change of a few characters in a single file with my around 250 notes. Now, its down to about 4 seconds.
This pull will only have an impact on linux users and is using https if needed. Check the code for further details if you like. In the long run, this could be implemented for other Clients if it proves to be useful, but for now its important to bring this out to the problematic linux desktop first.
Automated testing showed no errors.