lib/fs: Reduce memory usage in xattrs handling #9251
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reduces allocations, in number and in size, while getting extended attributes. This is mostly noticable when there is a large number of new files to scan and we're running with the default scanProgressInterval -- then a queue of files is built in-memory, and this queue includes extended attributes as part of file metadata. (Arguable it shouldn't, but that's a more difficult and involved change.) With 1M files to scan, each with one extended attribute, current peak memory usage looks like this: Showing nodes accounting for 1425.30MB, 98.19% of 1451.64MB total Dropped 1435 nodes (cum <= 7.26MB) Showing top 10 nodes out of 54 flat flat% sum% cum cum% 976.56MB 67.27% 67.27% 976.56MB 67.27% github.com/syncthing/syncthing/lib/fs.getXattr 305.44MB 21.04% 88.31% 305.44MB 21.04% github.com/syncthing/syncthing/lib/scanner.(*walker).walk.func1 45.78MB 3.15% 91.47% 1045.23MB 72.00% github.com/syncthing/syncthing/lib/fs.(*BasicFilesystem).GetXattr 22.89MB 1.58% 93.04% 22.89MB 1.58% github.com/syncthing/syncthing/lib/fs.listXattr 22.89MB 1.58% 94.62% 22.89MB 1.58% github.com/syncthing/syncthing/lib/protocol.(*PlatformData).SetXattrs 16MB 1.10% 95.72% 16.01MB 1.10% github.com/syndtr/goleveldb/leveldb/memdb.New After the change, it's this: Showing nodes accounting for 502.32MB, 95.70% of 524.88MB total Dropped 1400 nodes (cum <= 2.62MB) Showing top 10 nodes out of 91 flat flat% sum% cum cum% 305.43MB 58.19% 58.19% 305.43MB 58.19% github.com/syncthing/syncthing/lib/scanner.(*walker).walk.func1 45.79MB 8.72% 66.91% 68.68MB 13.09% github.com/syncthing/syncthing/lib/fs.(*BasicFilesystem).GetXattr 32MB 6.10% 73.01% 32.01MB 6.10% github.com/syndtr/goleveldb/leveldb/memdb.New 22.89MB 4.36% 77.37% 22.89MB 4.36% github.com/syncthing/syncthing/lib/fs.listXattr 22.89MB 4.36% 81.73% 22.89MB 4.36% github.com/syncthing/syncthing/lib/protocol.(*PlatformData).SetXattrs 15.35MB 2.92% 84.66% 15.36MB 2.93% github.com/syndtr/goleveldb/leveldb/util.(*BufferPool).Get 15.28MB 2.91% 87.57% 15.28MB 2.91% strings.(*Builder).grow (The usage for xattrs is reduced from 976 MB to 68 MB)
imsodin
reviewed
Dec 3, 2023
lib/fs/basicfs_xattr_unix.go
Outdated
| if size >= len(buf)/4*3 { | ||
| // The buffer is adequately sized (at least three quarters of it is | ||
| // used), return it as-is. | ||
| val := buf |
Member
There was a problem hiding this comment.
Don't we need slice according to size here?
Comment on lines
+107
to
+111
| // The buffer is larger than required, copy the data to a new buffer of | ||
| // the correct size. This avoids having lots of 1024-sized allocations | ||
| // sticking around when 24 bytes or whatever would be enough. | ||
| val := make([]byte, size) | ||
| copy(val, buf) |
Member
There was a problem hiding this comment.
This is the reason for the lower memory usage, right? And the pool is "just" an additional optimisation for allocations.
imsodin
approved these changes
Dec 4, 2023
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Dec 5, 2023
* main: lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) lib/scanner: Record inode change time for directories and symlinks (syncthing#9250) lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254) gui, man, authors: Update docs, translations, and contributors lib/fs: Better equality comparison in mtimefs cmd/stcrashreceiver: Add metrics for diskstore inventory cmd/stcrashreceiver: Minor cleanup, stricter file permissions cmd/stcrashreceiver: Add metrics for incoming reports cmd/ursrv: Add metrics for incoming reports gui, man, authors: Update docs, translations, and contributors gui: Specialize a special-purpose checkbox style (syncthing#9236) gui, man, authors: Update docs, translations, and contributors build: Support new nested namespaces in Weblate downloads lib/model: Acquire fmut lock in ensureIndexHandler (fixes syncthing#9234) (syncthing#9235) gui: Allow to translate "unknown device" (syncthing#9229)
calmh
added a commit
to cjc7373/syncthing
that referenced
this pull request
Dec 11, 2023
* main: (69 commits) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) lib/scanner: Record inode change time for directories and symlinks (syncthing#9250) lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254) gui, man, authors: Update docs, translations, and contributors lib/fs: Better equality comparison in mtimefs cmd/stcrashreceiver: Add metrics for diskstore inventory cmd/stcrashreceiver: Minor cleanup, stricter file permissions cmd/stcrashreceiver: Add metrics for incoming reports ...
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Dec 16, 2023
* main: (89 commits) build: Update quic-go (fixes syncthing#9287) lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288) lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reduces allocations, in number and in size, while getting extended attributes. This is mostly noticable when there is a large number of new files to scan and we're running with the default scanProgressInterval -- then a queue of files is built in-memory, and this queue includes extended attributes as part of file metadata. (Arguable it shouldn't, but that's a more difficult and involved change.)
With 1M files to scan, each with one extended attribute, current peak memory usage looks like this:
After the change, it's this:
(The usage for xattrs is reduced from 976 MB to 68 MB)