typst / ecow

Compact, clone-on-write vector and string.
Apache License 2.0
205 stars 16 forks source link

Ref counted types missing pointer comparison optimization for `eq` #35

Closed epage closed 8 months ago

epage commented 11 months ago

Arc<str> compares pointers first which normal reference equality checks do not perform.

See https://htmlpreview.github.io/?https://github.com/epage/string-benchmarks-rs/blob/master/runs/2023-10-10/eq/report/index.html for a benchmark showcasing the behavior from this

laurmaedje commented 11 months ago

Good catch, should be easy enough to fix.

laurmaedje commented 9 months ago

The PartialEq impl of Arc<T> only does this if T: Eq because of irreflexive impls, so it looks like this would need specialization for EcoVec. :/

For EcoString, it would work although I wonder whether it adds more overhead than it's worth for small strings.

I've pushed an implementation on a branch: https://github.com/typst/ecow/tree/ptr-eq

Kmeakin commented 9 months ago

IIRC there was a pull request to do the same optimization in PartialEq for [T] but was rejected because it actually caused a performance regression, but I can't find it right now.

I would recommend deferring to the str or [T] implementations, and then ecow can benefit from the same optimizations if the rust team decide it's advantageous

laurmaedje commented 8 months ago

Sounds good.