Skip to content

Use ecow as the representation for short, immutable strings#211

Merged
arcanis merged 8 commits intoyarnpkg:mainfrom
cijiugechu:box-git-range
Feb 9, 2026
Merged

Use ecow as the representation for short, immutable strings#211
arcanis merged 8 commits intoyarnpkg:mainfrom
cijiugechu:box-git-range

Conversation

@cijiugechu
Copy link
Copy Markdown
Contributor

@cijiugechu cijiugechu commented Jan 30, 2026

Extract a new crate that wraps ecow using the newtype pattern.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 30, 2026

⏱️ Benchmark Results

Metric Base Head Difference
Mean 2.835s 2.675s -5.67% ✅
Median 2.807s 2.676s -4.69% ✅
Min 2.643s 2.551s
Max 3.215s 3.038s
Std Dev 0.134s 0.091s
📊 Raw benchmark data

Base times: 3.136s, 3.215s, 3.171s, 2.749s, 2.887s, 2.643s, 2.718s, 2.776s, 2.904s, 2.813s, 2.756s, 2.884s, 2.765s, 2.838s, 2.796s, 2.801s, 2.826s, 2.866s, 2.782s, 2.881s, 2.750s, 2.779s, 2.658s, 2.719s, 2.710s, 2.841s, 2.793s, 2.869s, 2.822s, 2.916s

Head times: 3.038s, 2.725s, 2.689s, 2.647s, 2.562s, 2.689s, 2.578s, 2.746s, 2.593s, 2.647s, 2.715s, 2.705s, 2.673s, 2.683s, 2.654s, 2.662s, 2.725s, 2.679s, 2.551s, 2.678s, 2.731s, 2.745s, 2.635s, 2.723s, 2.579s, 2.584s, 2.732s, 2.556s, 2.661s, 2.658s


Benchmark: gatsby install-full-cold

@cijiugechu cijiugechu changed the title WIP: Reduce Range/Reference size via boxed Git variants WIP: Reduce Range/Reference size Jan 30, 2026
@cijiugechu cijiugechu changed the title WIP: Reduce Range/Reference size Use ecow as the representation for short, immutable strings Jan 30, 2026
@cijiugechu cijiugechu marked this pull request as ready for review January 30, 2026 07:17
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jan 30, 2026

Greptile Overview

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are a straightforward refactoring that moves code from one crate to another without modifying core logic. The newtype wrappers maintain the same API, all imports have been systematically updated, and tests have been appropriately modified to use the new types. The implementation preserves all serialization support and the DerefMut pattern that was already present in the original code.
  • No files require special attention

Important Files Changed

Filename Overview
packages/zpm-utils/src/ecow.rs New ecow wrapper with newtype pattern for EcoString and EcoVec, includes rkyv serialization support
packages/zpm-utils/src/lib.rs Added ecow module exports and ecow_crate re-export for macro usage
packages/zpm-primitives/src/ident.rs Replaced String with EcoString for Ident internal representation
packages/zpm-semver/src/range.rs Replaced String/Vec with EcoString/EcoVec in Range struct and updated all constructors
packages/zpm-semver/src/version.rs Replaced Vec with EcoVec for version RC components, updated next_rc to use make_mut()

Copy link
Copy Markdown
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Looks good, but shouldn't that rather be in zpm-utils ?

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment thread packages/zpm-utils/src/ecow.rs
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 4, 2026

Additional Comments (2)

packages/zpm-semver/src/version.rs
[P0] match last_char arm ordering makes '9'/'z' cases unreachable.

In next_immediate_spec, the arms '0'..'9' and 'a'..'z' appear before the more specific '9' and 'z' arms, so '9' and 'z' will always match the earlier ranges and never execute the intended wrap behavior (e.g., '9' -> 'a', 'z' -> "za"). This changes version bump semantics for RC strings that end with 9 or z.

Also appears in this same match block for the 'a'..'z' vs 'z' ordering.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm-semver/src/version.rs
Line: 155:169

Comment:
[P0] `match last_char` arm ordering makes `'9'`/`'z'` cases unreachable.

In `next_immediate_spec`, the arms `'0'..'9'` and `'a'..'z'` appear before the more specific `'9'` and `'z'` arms, so `'9'` and `'z'` will always match the earlier ranges and never execute the intended wrap behavior (e.g., `'9' -> 'a'`, `'z' -> "za"`). This changes version bump semantics for RC strings that end with `9` or `z`.

Also appears in this same match block for the `'a'..'z'` vs `'z'` ordering.

How can I resolve this? If you propose a fix, please make it concise.

packages/zpm-semver/src/version.rs
[P1] Incrementing RC string uses as u8 on char, which is incorrect for non-ASCII.

all_but_last_str.push((last_char as u8 + 1) as char) will not compile on stable Rust (char can’t be cast to u8 directly), and even if adjusted it would be wrong for non-ASCII digits/letters. Given the function already constrains input to ASCII ranges, this should use last_char as u8 only after converting via as u32/u8::try_from, or better operate on byte literals.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/zpm-semver/src/version.rs
Line: 164:166

Comment:
[P1] Incrementing RC string uses `as u8` on `char`, which is incorrect for non-ASCII.

`all_but_last_str.push((last_char as u8 + 1) as char)` will not compile on stable Rust (`char` can’t be cast to `u8` directly), and even if adjusted it would be wrong for non-ASCII digits/letters. Given the function already constrains input to ASCII ranges, this should use `last_char as u8` only after converting via `as u32`/`u8::try_from`, or better operate on byte literals.

How can I resolve this? If you propose a fix, please make it concise.

@cijiugechu
Copy link
Copy Markdown
Contributor Author

Looks good, but shouldn't that rather be in zpm-utils ?

Done in the last revision.

@arcanis arcanis merged commit 0c26f8b into yarnpkg:main Feb 9, 2026
12 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