Make Atom generic over the set of static strings by SimonSapin · Pull Request #178 · servo/string-cache

SimonSapin · 2016-10-27T08:14:39Z

The new string_cache_codegen crate is intended to be used by downstream users in their build scripts. Provided a list of static strings, it generates code that defines:

A type alias for Atom<_> with the type parameter set,
A macro like the former atom! macro.

Based on #136, original work by @aidanhs.

Fixes #22, fixes #136, closes #83.

r? @nox

This change is

selectors doesn’t use it anymore.

Prepare for adding a type parameter that derive would require bounds on.

We’re about to introduce a trait name StaticAtomSet.

aidanhs · 2016-10-27T11:44:53Z

Oh boy, I'm excited! No worries about taking so long, I understand it can take a while for the stars to align and get time. Some thoughts:

the readme for string-cache-codegen is no longer present. Although there are docs, I tend to think it's nice to have something in a github readme as well (potentially even at the top level)
the 'test' crate in a subfolder of string-cache-codegen is no longer present. I can't see string-cache-codegen ever breaking, but the test seems like a useful thing to have
good catch on the 'hashing twice' thing. It's been a while since I wrote the code, but I think the trade-off is that you can either hash once and end up storing the strings for identical dynamic atoms twice (because the hash is based off the static atom set hash), or you can hash twice (the second time with the fixed 'dynamic set' key) and be guaranteed to only store each string once. On reflection, given that applications are likely only going to have one kind of atom the latter is not really a big win.

I'll actually try it out a bit later.

SimonSapin · 2016-10-27T12:37:19Z

Oh, good point about using different hash functions in the one dynamic atom hash table. I had not thought of that.

So now I'm not sure it's the right thing to do :) I'll think on it a bit more.

SimonSapin · 2016-10-27T15:45:53Z

After chatting with @nox I think we’re gonna keep things as-is now regarding hashing. We can always change it later (without even breaking any public API).

Regarding testing, the string_cache crate’s own build.rs uses string_cache_codegen to generate a TestAtom, which is used in unit tests. Since this matches the expected usage of the codegen crate, I believe a separate testing crate is not needed.

I’ve pushed another commit that extends the README with some usage examples.

aidanhs · 2016-10-28T00:47:26Z

Ok, I've tried it out and it seems to work fine for me and the examples look good to me. It's a little sad it can't use phf generated sets directly, but I can see that's tricky given the desire to want to have direct access to the hash value without recomputing. Maybe someday.

Nothing else from me I don't think 👍

SimonSapin · 2016-10-28T05:41:25Z

string_cache::PhfSet is a lot like phf::Set<&'static str>, just with more stable internals. (See e.g. this warning.)

I didn’t make it expose any public functionality to keep small the set of API we’ll need to maintain and because I didn’t see a use for it, but it could. What would you like to do with the set, and why?

aidanhs · 2016-10-31T12:52:23Z

It's more a general feeling that it'd be nice to use phf_codegen (and therefore phf_sets) directly rather than recreating them. As you note, given that parts of the api are not considered public it'd be a bit risky to depend on them for functionality. Perhaps the long term solution is to negotiate with phf to expose the bits that would be useful for string-cache, but I wouldn't want that to come before merging this PR.

Manishearth

Overall lgtm. What's the plan for namespaces?

Manishearth · 2016-11-01T16:19:29Z

build.rs

-    slice
+    string_cache_codegen::AtomType::new("atom::tests::TestAtom", "test_atom!")
+        .atoms(&[
+            "a", "b", "address", "area", "body", "font-weight", "br","html", "head", "id",


nit: space after br

Manishearth · 2016-11-01T16:21:44Z

examples/summarize-events/src/main.rs

 mod shared;

-use string_cache::Atom;
+use string_cache::DefaultAtom;


we should probably use DefaultAtom as Atom consistently across the examples

Manishearth · 2016-11-01T16:29:05Z

src/atom.rs

-                }
+    fn from(string_to_add: Cow<'a, str>) -> Self {
+        let static_set = Static::get();
+        let hash = phf_shared::hash(&*string_to_add, static_set.key);


why not continue to use get_index_or_hash()?

It would have to be a method of a public trait, and thus changing it (if we need to in the future) would be a breaking change.

It’s going into various atom types in html5ever and Servo.

SimonSapin · 2016-11-01T17:00:14Z

The plan for namespaces is to have them in a new html5ever-atoms crate in the html5ever repo: https://github.com/servo/html5ever/compare/string-cache-up#diff-04339865c75d55a398b31d0a84d21c56R42

Namespace and Prefix there are both type aliases of Atom<SomeStaticSet> generated with string_cache_codegen. The ns!() macro (which expands to a Namespace value based on the prefix) is generated separately by the build script.

Manishearth · 2016-11-01T19:05:04Z

r=me , unless @nox wants to have a look

SimonSapin · 2016-11-02T13:21:18Z

@bors-servo r=Manishearth

bors-servo · 2016-11-02T13:21:20Z

📌 Commit 4528d77 has been approved by Manishearth

bors-servo · 2016-11-02T13:21:30Z

⚡ Test exempted - status

@aidanhs

Make Atom generic over the set of static strings The new `string_cache_codegen` crate is intended to be used by downstream users in their build scripts. Provided a list of static strings, it generates code that defines: - A type alias for `Atom<_>` with the type parameter set, - A macro like the former `atom!` macro. Based on #136, original work by @aidanhs. Fixes #22, fixes #136, closes #83. r? @nox  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/string-cache/178)

aidanhs · 2016-11-02T17:59:09Z

🎆

SimonSapin mentioned this pull request Oct 27, 2016

Use new string_cache_codegen crate for static atoms #136

Closed

SimonSapin and others added 12 commits October 27, 2016 11:55

Use more compact TOML syntax.

90bdc78

Breaking changes are coming.

8581617

Remove BorrowedAtom.

0c82b2c

selectors doesn’t use it anymore.

Remove macro hack rendered unneeded by $crate.

fef1fa6

Remove namespaces. They’re going into htmlever.

dbf6ad7

Stop using derive.

135c895

Prepare for adding a type parameter that derive would require bounds on.

Rename StaticAtomSet struct to PhfStrSet.

046b044

We’re about to introduce a trait name StaticAtomSet.

Add a type parameter to Atom.

367bf9f

Make StaticAtomSet impls provide a PhfStrSet.

0ad43ea

Inline PhfStrSet methods.

c3eac36

Flatten the src/atom directory

684191e

Add EmptyStaticAtomSet and DefaultAtom.

fabedb0

SimonSapin force-pushed the static-sets branch from fe9d8f7 to d5b1001 Compare October 27, 2016 09:56

SimonSapin force-pushed the static-sets branch from b54d9d9 to 0a81c4d Compare October 27, 2016 15:43

Manishearth approved these changes Nov 1, 2016

View reviewed changes

SimonSapin and others added 7 commits November 1, 2016 17:56

Make examples/* crates build again.

e9a82f7

Link to docs.rs rather than doc.servo.org.

4a6d5f1

Add and use new string-cache-codegen crate

f054f9b

Remove the big static atom list. Fix #22

570fea0

It’s going into various atom types in html5ever and Servo.

Make the Default impl use a static atom.

06b2116

Add usage example in README.

1d256d0

Note in toml files to also update README

4528d77

SimonSapin force-pushed the static-sets branch from ce286ac to 4528d77 Compare November 1, 2016 16:57

bors-servo merged commit 4528d77 into master Nov 2, 2016

SimonSapin deleted the static-sets branch November 2, 2016 13:23

SimonSapin mentioned this pull request Nov 2, 2016

Add "associated data" to support namespaces #29

Closed

Conversation

SimonSapin commented Oct 27, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aidanhs commented Oct 27, 2016

Uh oh!

SimonSapin commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSapin commented Oct 27, 2016

Uh oh!

aidanhs commented Oct 28, 2016

Uh oh!

SimonSapin commented Oct 28, 2016

Uh oh!

aidanhs commented Oct 31, 2016

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Manishearth Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

SimonSapin Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

Manishearth Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

SimonSapin Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

Manishearth Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

SimonSapin Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

SimonSapin commented Nov 1, 2016

Uh oh!

Manishearth commented Nov 1, 2016

Uh oh!

SimonSapin commented Nov 2, 2016

Uh oh!

bors-servo commented Nov 2, 2016

Uh oh!

bors-servo commented Nov 2, 2016

Uh oh!

aidanhs commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SimonSapin commented Oct 27, 2016 •

edited by larsbergstrom

Loading

SimonSapin commented Oct 27, 2016 •

edited

Loading