Skip to content

atomic_writer tiny cleanups - #9807

Merged
hsbt merged 3 commits into
ruby:masterfrom
nobu:atomic_writer-tiny-cleanups
Sep 3, 2026
Merged

atomic_writer tiny cleanups#9807
hsbt merged 3 commits into
ruby:masterfrom
nobu:atomic_writer-tiny-cleanups

Conversation

@nobu

@nobu nobu commented Aug 25, 2026

Copy link
Copy Markdown
Member

What was the end-user or developer problem that led to this PR?

Trivial performance improvements.

What is your fix for the problem, implemented in this PR?

  • Use the dedicated method String#scrub!.
  • Use the FD-based instance methods.

Make sure the following tasks are checked

nobu added 2 commits August 25, 2026 12:46
Since the base `string` is a valid encoding, there should be no other
invalid sequences in `sliced`.
Operations on an already open file are faster and safer than those
based on the path name.
Comment thread lib/rubygems/util/atomic_file_writer.rb Outdated
if string.valid_encoding?
sliced = sliced.byteslice(0, sliced.bytesize - 1) until sliced.valid_encoding?
end
sliced.scrub!("") if string.valid_encoding?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

scrub!("") raises Encoding::CompatibilityError for ASCII-incompatible encodings (e.g. UTF-16LE), even when the string is fully valid, because the replacement literal is UTF-8 under frozen_string_literal.

The previous loop handled those encodings gracefully.

A UTF-8 replacement makes String#scrub! raise
Encoding::CompatibilityError on an ASCII-incompatible receiver such as
UTF-16LE, where the loop trimmed the tail fine. It is also slower than
the loop for the common case of an ASCII name that needs no truncation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@hsbt

hsbt commented Sep 3, 2026

Copy link
Copy Markdown
Member

I dropped the scrub! change because making it encoding-safe leaves performance essentially unchanged.

@hsbt
hsbt merged commit ac5e04a into ruby:master Sep 3, 2026
106 checks passed
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