Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[smallvec] Use SmallVec for some internal scratch vectors #127

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

jfkthame
Copy link
Contributor

We have some temporary vectors used during processing that will usually only hold a few entries. By using TinyVec here, we can avoid extra heap allocations in common cases.

(In Gecko's usage, I'm seeing around 4% improvement on the bidi- resolution subtest of perf_reftest_singletons with this change.)

@Manishearth
Copy link
Member

This crate deliberately has zero mandatory deps: it is a transitive dependency of url which gets used by most http crates, and is at the bottom of most deptrees.

I'm fine with a quick (safe) singlepurpose enum impl of tinyvec being written here, or with making this an optional dep. I slightly prefer the former.

We have some temporary vectors used during processing that will
usually only hold a few entries. By using SmallVec here, we can
avoid extra heap allocations in common cases.

(In Gecko's usage, I'm seeing around 4% improvement on the bidi-
resolution subtest of perf_reftest_singletons with this change.)
@jfkthame
Copy link
Contributor Author

This crate deliberately has zero mandatory deps: it is a transitive dependency of url which gets used by most http crates, and is at the bottom of most deptrees.

I'm fine with a quick (safe) singlepurpose enum impl of tinyvec being written here, or with making this an optional dep. I slightly prefer the former.

Fair enough. For now I'm inclined to go with making it an optional dependency, as that seems quite a bit simpler than providing our own implementation of it, though if anyone wants to do that as a future enhancement I'd be fine with it.

I'll (force-)push a new version where it's optional. (I've also switched it from tinyvec to smallvec, which is equivalent for this purpose but better from my POV because Gecko already has it as a dependency.)

@jfkthame jfkthame changed the title [tinyvec] Use TinyVec for some internal scratch vectors [smallvec] Use SmallVec for some internal scratch vectors Feb 21, 2024
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Alright. Please file a followup for adding an in-tree representation.

I don't think it's going to be that complicated, it's basically an enum between an array and a vec and the only operation we care about is push.

@Manishearth Manishearth merged commit d43c644 into servo:master Feb 21, 2024
7 checks passed
@jfkthame jfkthame deleted the tinyvec branch February 26, 2024 11:09
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