Skip to content

Conversation

@pczarn
Copy link
Contributor

@pczarn pczarn commented Nov 13, 2020

Closes #6

The StringInterner::get_or_intern has an unnecessary call to to_string which allocates. There is no clean workaround, AFAIK. We would need an improved Entry API. See also ideas such as entry_or_clone: Extend entry API to work on borrowed keys rust-lang/rfcs#1769

Yet another nice-to-have thing would be a stable custom allocation story. With custom allocators, we could easily replace the meat of the string_interner crate. Each interner would hold its own allocator, thereby gaining top performance.

example: StringInterner
Cargo.toml Outdated
categories = ["data-structures", "caching"]

[features]
default = ["indexmap"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the feature be on by default, or not?

Copy link
Owner

Choose a reason for hiding this comment

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

No extra dependencies by default, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Manishearth fixed

@pczarn
Copy link
Contributor Author

pczarn commented Nov 15, 2020

I did the right Cargo.toml, travis and README changes for the string_interner example. (in the last commit)

@pczarn pczarn force-pushed the frozen_index_map_set branch from a38f8b2 to 0a46740 Compare November 15, 2020 09:45
@Manishearth
Copy link
Owner

Manishearth commented Dec 2, 2020

Looks great, thank you! Sorry for the delay in reviewing!

@Manishearth Manishearth merged commit f9970bc into Manishearth:master Dec 2, 2020
@Manishearth
Copy link
Owner

Feel free to open a PR that releases version 1.4

@zertosh zertosh mentioned this pull request Mar 6, 2021
@zertosh
Copy link
Contributor

zertosh commented Mar 6, 2021

@Manishearth, I took the liberty of putting up the bump PR (#11) because I would really like to use FrozenIndexSet.

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.

(Optional?) (Frozen)Index{Map,Set}

3 participants