Skip to content

perf!: use Cow to avoid OsString allocations#487

Merged
aawsome merged 2 commits intorustic-rs:mainfrom
tilpner:node-name-cow
Mar 6, 2026
Merged

perf!: use Cow to avoid OsString allocations#487
aawsome merged 2 commits intorustic-rs:mainfrom
tilpner:node-name-cow

Conversation

@tilpner
Copy link
Contributor

@tilpner tilpner commented Mar 5, 2026

This PR eliminates many temporary allocations from unnecessary uses of OsString (they remain necessary for TreeIterator without lending iterators).

Previously, filename unescaping accounted for 43% of all total allocations, even in cases where there was nothing to unescape. This is avoided by checking for the presence of \ before, and using the original name if none was found.

comp_to_osstr was only responsible for 0.1% of allocations.

Results

The following measurements are not from a proper benchmarking setup, but merely repeated low-delta backups with heaptrack (without LTO and with raised codegen-units, on a laptop):

Total allocations reduced from 173M to 98M in my case.

These changes did not lower peak RSS significantly (4.7GiB to 4.6GiB, probably noise or slightly reduced fragmentation), which is expected for mostly temporary allocations.

Using the global allocator with glibc 2.40, there wasn't much (if any) walltime improvement (284s to 274s, measured without heaptrack), but it might be more significant on platforms with weaker allocators like musl.

@aawsome aawsome changed the title perf: use Cow to avoid OsString allocations perf!: use Cow to avoid OsString allocations Mar 6, 2026
@aawsome
Copy link
Member

aawsome commented Mar 6, 2026

About the TreeIterator: I think this is one part which anyway needs some overwork: This is barely readable and too complex to understand and enhance. I wanted to implement features like allowing to use multiple different sources for a single snapshot (e.g. stdin and a local path) or to add additional sources (e.g. remote sources or using a tar as source) and almost always this complex "monster" was in the way and prevented a simple extension. (Ok, plus the parallelization used enforcing Send+Sync for many things). So IMO it's OK to keep the OsString here and tackle this when overworking the general tree handling for backups.

@aawsome
Copy link
Member

aawsome commented Mar 6, 2026

And another thing: There is #390 which moves away from platform-dependent OsString/OsStr. But this anyway needs rebasing and some extra work; so I think we can first merge this quite small performance optimization!

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot @tilpner

@aawsome aawsome enabled auto-merge March 6, 2026 13:50
@aawsome aawsome added this pull request to the merge queue Mar 6, 2026
Merged via the queue into rustic-rs:main with commit 00b93d9 Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants