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

We should start caching strings, instead of copying them all around #11

Closed
Yoric opened this issue Feb 25, 2016 · 6 comments
Closed

We should start caching strings, instead of copying them all around #11

Yoric opened this issue Feb 25, 2016 · 6 comments

Comments

@Yoric
Copy link
Collaborator

Yoric commented Feb 25, 2016

Many Strings will appear several times (e.g. tags). To avoid copying them, we should use a mechanism of atoms, that makes sure we only have one instance of the String in memory at any given time.

@Yoric
Copy link
Collaborator Author

Yoric commented Mar 3, 2016

@SimonSapin has developed a crate for this. We should start using it for:

  • Id<T>;
  • tags.

Given that the crate is pretty much not documented, this may mean documentation work, too.

@SimonSapin
Copy link

(I’m more or less the maintainer of that crate these days, but it was developed by @ glennw and @ kmcallister.)

Yes, docs need work. In the meantime, the main APIs are Atom::from(&str) -> Atom, Atom dereferences to str, and comparing or hashing atoms is fast (it compares/hashes u64’s).

@Yoric Yoric changed the title Consider replacing strings with Atoms We should start caching strings, instead of copying them all around Mar 3, 2016
@azasypkin
Copy link
Member

Hey @SimonSapin,

I see that string_cache crate includes a huge list of predefined atoms that look specific to Servo [1].

Is it included in final binary for every consumer of this crate? If so, is it possible to override it somehow?

Thanks!

[1] https://github.com/servo/string-cache/blob/master/src/static_atom_list.rs

@SimonSapin
Copy link

@azasypkin Currently yes they’re included and no you can’t change them. There are plans to make Atom generic so that each user-defined "kind" of atom has its own list of static strings: servo/string-cache#22 and servo/string-cache#136

@azasypkin
Copy link
Member

Okay, got it, thanks!

Yoric pushed a commit that referenced this issue Mar 6, 2016
Prior to this patch, empty variants such as "OpenClosed::On" were
serialized as "On: []", which is quite surprising. This patch
overrides the default behavior of OpenClosed and OnOff to make sure
that the empty variants are serialized simply as strings.

For this purpose, the patch introduces a dependency to
`serde_utils`. If this proves troublesome, we may be able to
eventually get rid of this dependency.
Yoric pushed a commit that referenced this issue Mar 6, 2016
This patch introduces some documentation/doctests on Value. More still
needs to be written.
Yoric pushed a commit that referenced this issue Mar 8, 2016
@Yoric
Copy link
Collaborator Author

Yoric commented Mar 10, 2016

Well, first part has landed.

@Yoric Yoric closed this as completed Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants