Having a separate mutex for the three or four instructions needed to
fetch and increment nextID means the overhead exceeds the cost of this
operation. nextID is now handled inside the critical section for
awaiting instead, while the more expensive channel creation has been
moved outside it.
This is mostly a simplification, though it may have minor performance
benefits in some situations. The single-threaded sender benchmark shows
no significant difference:
name old speed new speed delta
RequestsRawTCP-8 55.3MB/s ± 7% 56.6MB/s ± 6% ~ (p=0.190 n=10+10)
RequestsTLSoTCP-8 20.5MB/s ±20% 20.8MB/s ± 8% ~ (p=0.604 n=10+9)
Staggered File Versioning used to have its own cleanInterval that
controlled how often file versions were cleaned. Nowadays, there is a
seperate setting called cleanupIntervalS responsible for the cleanup,
which applies to all File Versioning (except External). Thus, remove the
unneeded code and don't set the param up on new folders anymore.
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
* lib/protocol: Require at least 3.125% savings from compression
The new lz4 library doesn't need its output buffer to be the maximum
size, unlike the old one (which would allocate if it weren't). It can
take a buffer that is of a smaller size and will report if compressed
data can fit inside the buffer (with a small chance of reporting a false
negative). Use that property to our advantage by requiring compressed
data to be at most n-n/32 = .96875*n bytes long for n input bytes.
* lib/protocol: Remove unused receivers
To make DeepSource happy.
* lib/protocol: Micro-optimize lz4Compress
Only write the length if compression was successful. This is a memory
write, so the compiler can't reorder it.
Only check the return value of lz4.CompressBlock. Length-zero inputs
are always expanded by LZ4 compression (the library documents this),
so the check on len(src) isn't needed.
* lib/model: Remove bogus fields from connections API endpoint.
Switch the returned data type for the /rest/system/connections element
"total" to use only the Statistics struct. The other fields of the
ConnectionInfo struct are not populated and misleading.
* Lowercase JSON field names.
* lib/model: Get rid of ConnectionInfo.MarshalJSON().
It was missing the StartedAt field from the embedded Statistics
struct. Just lowercasing the JSON attribute names can be done just as
easily with annotations.
* lib/model: Remove bogus startedAt field from totals.
Instead of using the Statistics type with one field empty, just switch
to a free-form map with the three needed fields.
* cmd/syncthing: Remove unnecessary function arguments.
The openGUI() function does not need a device ID to work, and there is
only one caller anyway which uses EmptyDeviceID.
The loadOrDefaultConfig() function is always called with the same
dummy values.
* cmd/syncthing: Avoid misleading info messages from monitor process.
In order to check whether panic reporting is enabled, the monitor
process utilizes the loadOrDefaultConfig() function. In case there is
no config file yet, info messages may be logged during creation if the
config Wrapper, which is discarded immediately after.
Stop using the DefaultConfig() utility function from lib/syncthing and
directly generate a minimal config instead to avoid these.
Add comments to loadOrDefaultConfig() explaining its limited purpose.
* cmd/syncthing/generate: Always write updated config file.
Previously, an existing config file was left untouched unless either
of the --gui-user or --gui-password options was given. Remove that
condition and simplify the checking code.
* lib/config: Factor out ProbeFreePorts().
* cmd/syncthing: Add option --skip-port-probing.
Applies to both the "generate" and "serve" subcommands, as well as the
deprecated --generate option, just as the --no-default-folder flag.
When pathSep is a constant, the compiler precomputes pathSep+pathSep and
".."+pathSep instead of emitting function calls to compute "//" and
"../". Benchmark results in lib/osutil:
name old time/op new time/op delta
TraversesSymlink-8 8.86µs ± 3% 8.53µs ± 4% -3.79% (p=0.000 n=18+20)
name old alloc/op new alloc/op delta
TraversesSymlink-8 1.06kB ± 0% 1.06kB ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
TraversesSymlink-8 15.0 ± 0% 15.0 ± 0% ~ (all equal)
The locking protocol in nat.Mapping was racy:
* Mapping.addressMap RLock'd, but then returned a map shared between
caller and Mapping, so the lock didn't do anything.
* Operations inside Service.{verifyExistingMappings,acquireNewMappings}
would lock the map for every update, but that means callers to
Mapping.ExternalAddresses can be looping over the map while the
Service methods are concurrently modifying it. When the Go runtime
detects that happening, it panics.
* Mapping.expires was read and updated without locking.
The Service methods now lock the map once and release the lock only when
done.
Also, subscribers no longer get the added and removed addresses, because
none of them were using the information. This was changed for a previous
attempt to retain the fine-grained locking and not reverted because it
simplifies the code.
Accept a subcommand as an alternative to the --generate option. It
accepts a custom config directory through either the --home or
--config options, using the default location if neither is given.
Add the options --gui-user and --gui-password to "generate", but not
the "serve --generate" option form. If either is given, an existing
config will not abort the command, but rather load, modify and save it
with the new credentials. The password can be read from standard
input by passing only a single dash as argument.
Config modification is skipped if the value matches what's already in
the config.
* cmd/syncthing: Utilize lib/locations package in generate().
Instead of manually joining paths with "magic" file names, get them
from the centralized locations helper lib.
* cmd/syncthing: Simplify logging for --generate option.
Visible change: No more timestamp prefixes.
What hash is used to store the password should ideally be an
implementation detail, so that every user of the GUIConfiguration
object automatically agrees on how to handle it. That is currently
distribututed over the confighandler.go and api_auth.go files, plus
tests.
Add the SetHasedPassword() / CompareHashedPassword() API to keep the
hashing method encapsulated. Add a separate test for it and adjust
other users and tests. Remove all deprecated imports of the bcrypt
package.
LoadOrGenerateCertificate() takes two file path arguments, but then
uses the locations package to determine the actual path. Fix that
with a minimally invasive change, by using the arguments instead.
Factor out GenerateCertificate().
The only caller of this function is cmd/syncthing, which passes the
same values, so this is technically a no-op.
* lib/tlsutil: Make storing generated certificate optional. Avoid
temporary cert and key files in tests, keep cert in memory.
Consistently use double dashes and fix typos -conf, -data-dir and
-verify.
Applies also to tests running the syncthing binary for consistency.
* Fix mismatched option name --conf in cli subcommand.
According to the source code comments, the cli option flags should
mirror those from the serve subcommand where applicable. That one is
actually called --config though.
* cli: Fix help text option placeholders.
The urfave/cli package uses the Value field of StringFlag to provide a
default value, not to name the placeholder. That is instead done with
backticks around some part of the Usage field.
* cli: Add missing --data flag in subcommand help text.
The urfave/cli based option parsing uses a fake flags collection to
generate help texts matching the used global options. But the --data
option was omitted from it, although it is definitely required when
using --config as well. Note that it cannot just be ignored, as some
debug stuff actually uses the DB:
syncthing cli --data=/bar --config=/foo debug index dump
By truncating time.Time to an int64 nanosecond count, we lose the
ability to precisely order timestamps before 1678 or after 2262, but we
gain (linux/amd64, Go 1.17.1):
name old time/op new time/op delta
JobQueuePushPopDone10k-8 2.85ms ± 5% 2.29ms ± 2% -19.80% (p=0.000 n=20+18)
JobQueueBump-8 34.0µs ± 1% 29.8µs ± 1% -12.35% (p=0.000 n=19+19)
name old alloc/op new alloc/op delta
JobQueuePushPopDone10k-8 2.56MB ± 0% 1.76MB ± 0% -31.31% (p=0.000 n=18+13)
name old allocs/op new allocs/op delta
JobQueuePushPopDone10k-8 23.0 ± 0% 23.0 ± 0% ~ (all equal)
Results for BenchmarkJobQueueBump are with the fixed version, which no
longer depends on b.N for the amount of work performed. rand.Rand.Intn
is cheap at ~10ns per iteration.
Like Windows and Mac, Android is also an interactive operating system.
On top of that, it usually runs on much slower hardware than the other
two. Because of that, it makes sense to limit the number of hashes used
by default there too.
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Registry.Get used a full sort to get the minimum of a list, and the sort
was broken because util.AddressUnspecifiedLess assumed it could find out
whether an address is IPv4 or IPv6 from its Network method. However,
net.(TCP|UDP)Addr.Network always returns "tcp"/"udp".
The current detection is flawed, because it looks for a few specific
file systems like "msdos" or "fat" to set the mtime window, while in
reality Android seems to report names like "fuseblk", which can stand
for fat, ext4, or even f2fs.
At the moment, we set the mtime window only for a few known names used
for the fat filesystem. With this change, we take a safer approach of
always setting the time window unless we explicitly detect file systems
like ext2/ext3/ex4, which are known not to experience issues with moving
timestamps on Android.
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
