Skip to content

Backport v2’s retain implementation to v1 - #596

Merged
alejandro-vaz merged 5 commits into
servo:v1from
astral-sh:charlie/codex-v1-compact
Sep 12, 2026
Merged

Backport v2’s retain implementation to v1#596
alejandro-vaz merged 5 commits into
servo:v1from
astral-sh:charlie/codex-v1-compact

Conversation

@charliermarsh

@charliermarsh charliermarsh commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR applies the existing v2 compaction approach to v1's retain: leave the initial retained prefix in place, then move each survivor into the next hole instead of swapping it with a discarded element. Removed elements are dropped during the scan, and a guard restores the untouched tail if the predicate or a destructor panics.

I benchmarked this on Linux (x86), and retaining half the input took 40-60% less time for small integer vectors, 16-21% less time for 4,096 integers, and 24-38% less for 4,096 larger elements:

Workload Element size Before After Paired time change
Keep half, 16 elements 8 B 39.33 ns 22.10 ns −45.5% to −42.0%
Keep half, 17 elements 8 B 46.95 ns 20.97 ns −59.9% to −54.1%
Keep half, 4,096 elements 8 B 17.205 µs 14.247 µs −21.0% to −15.9%
Keep half, 4,096 elements 128 B 31.931 µs 21.497 µs −37.8% to −23.6%
Keep all, 16 elements 8 B 39.79 ns 12.46 ns −69.8% to −67.6%
Keep all, 17 elements 8 B 38.71 ns 9.93 ns −80.3% to −73.0%
Keep all, 4,096 elements 128 B 19.722 µs 14.650 µs −29.7% to −7.3%
Keep none, 16 elements 8 B 40.60 ns 17.24 ns −57.6% to −55.8%
Keep none, 17 elements 8 B 40.94 ns 18.55 ns −55.1% to −54.4%
Keep none, 4,096 elements 128 B 18.427 µs 17.594 µs −7.0% to +13.1%
Unchanged Vec control: keep half, 4,096 elements 128 B 21.275 µs 21.486 µs −9.6% to +10.8%

(I included some native benchmarks but removed in a follow-up commit; you can see them in the PR history.)

Comment thread tests/retain.rs Outdated
Comment thread src/tests.rs Outdated
@alejandro-vaz

alejandro-vaz commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

can you paste the table with performance changes??

just to have it explicit on the PR

@charliermarsh

Copy link
Copy Markdown
Contributor Author

Done!

@alejandro-vaz

Copy link
Copy Markdown
Collaborator

thanks @charliermarsh

@alejandro-vaz
alejandro-vaz merged commit d0556cb into servo:v1 Sep 12, 2026
7 checks passed
@alejandro-vaz alejandro-vaz mentioned this pull request Sep 12, 2026
5 tasks
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