cmd/syncthing, lib/config, lib/osutil: Lower process priority (fixes #4628) #4675
Conversation
lib/osutil/lowprio_linux.go
Outdated
| // group variants of Setpriority etc to affect all of our threads in one | ||
| // go. If this fails, bail, so that we don't affect things we shouldn't. | ||
| if err := syscall.Setpgid(0, 0); err != nil { | ||
| return |
There was a problem hiding this comment.
I don't see the point, it's just called once from main and we'd need to remember the error in another package variable to be able to return it again etc
There was a problem hiding this comment.
Yeah, I was just pointing out in case other Go users use it. Also, with errors being returned, sync.Once makes more sense (as I assume doing pgrp twice will fail).
cmd/syncthing/main.go
Outdated
| @@ -915,6 +915,11 @@ func syncthingMain(runtimeOptions RuntimeOptions) { | |||
|
|
|||
| cleanConfigDirectory() | |||
|
|
|||
| if cfg.Options().SetLowPriority { | |||
| // This is best effort and we ignore failures. | |||
| osutil.SetLowPriority() | |||
There was a problem hiding this comment.
We could atleast log failures.
|
Would it make sense to make this like a value you can adjust? Default being like 3, so people can go up and down? Perhaps someone is running syncthing as a service. |
|
I'd really not want to add too many knobs to this because then we'll end up having to expose all the scheduler classes for iopriority, the various names of the priority classes in windows or the bitmasks, etc. I think we should just do something sane by default, and if someone wants something better they can disable our stuff by config and do whatever they please at launch time. |
|
Is it worth logging failures, under some debug level perhaps? |
|
Logging failures added... As warning level no less. Is that too aggressive? Maybe have it like that for the RC and see what drops in? My reasoning for warning is that once we've decided the Syncthing should not be a CPU & I/O resource hog it's worth warning the user that it might be, under the assumption that this is a very rare failure... |
lib/osutil/lowprio_windows.go
Outdated
| } | ||
| defer syscall.CloseHandle(handle) | ||
|
|
||
| _, _, err := syscall.Syscall(uintptr(setPriorityClass), uintptr(handle), belowNormalPriorityClass, 0, 0) |
There was a problem hiding this comment.
Why is the ioprio return value handling different from windows?
There was a problem hiding this comment.
It's not, any more, I think
|
(A note on copyright: in my understanding, the date in the copyright is the earliest year at which you assert copyright over the work, not the latest year. It says "I wrote this code in 2017, so anything which appears since then and isn't written by me is a derivative work". If you update the copyright year to 2018, you're more or less relinquishing your claim to have written the work in 2017, so if another copy pops up copyrighted 2017, your claim to hold copyright looks a little shaky...). |
|
@st-review merge |
st-review
added
the
pr-merged
st-review
added
the
frozen-due-to-age
Purpose
Be nicer to the rest of the system by default.
Adds a new option
setLowPrioritywhich is true by default. If set to false we do nothing to change our priority on startup, like before.Testing
I've tested this on Mac, Linux and FreeBSD and see the desired result on all of them: all threads in the final syncthing process (not the monitor) become niceness level 9.
I have not inspected the result on Windows... I don't have a Windows VM on my laptop right now, I'd really appreciate if someone ran the resulting binary and checked if it did what it's supposed to and didn't, like, crash or something...