Use new string_cache_codegen crate for static atoms #136

aidanhs · 2016-01-29T14:49:05Z

Test fails because the string-cache-codegen subfolder needs to be released to crates.io

All tests pass down to rust 1.4.0.

Some notes on string_cache:

string_cache now exposes the phf crate, otherwise consumers would need to depend on phf themselves and add extern crate phf when doing codegen
it's "backwards compatible" assuming you're not touching STATIC_ATOM_SET directly or calling any methods on it yourself - atom! etc will still work for all of the servo-specific atoms
SomeAtom::from (where SomeAtom is Atom or some other type generated by string_cache_codegen) will either return a static atom from the static string set corresponding to that type, or will insert a dynamic atom into a dynamic string cache shared among all atom kinds
you will not be allowed to compare atoms of different kinds - I consider this a good thing as it wouldn't work for static atoms

Some specific notes on string_cache_codegen:

as described in the documentation, generated macros must be at the root of the crate or they won't work
it uses a bit of a hack with how phf_codegen works to use a custom location for the phf crate (i.e. at string_cache::shared::phf). The motivation was to not require users of string_cache_codegen to depend on phf themselves.
I've copied some bits over from string_cache rather than listing it as a dependency. This allows usage of string_cache_codegen within the build.rs of string_cache itself

If you want to try it out, you just need to checkout my branch and add path = "string-cache-codegen" to the end of the root Cargo.toml. You can now run cargo build in the repo root (to test new static atom generation for string_cache) and the string_cache_codegen subfolder (to check it builds). You can also run cargo test in the root folder and string_cache_codegen/test subfolders and it should all work.

SimonSapin · 2016-01-29T15:15:32Z

Why a separate repository? These crates will likely need to be updated together.

aidanhs · 2016-01-29T15:19:49Z

I can do that if you prefer, it's up to you. My main hesitation was described in #22 (comment)

Since there's a delay, I have a question for the eventual submission - where will the new string_cache_codegen crate live? PHF keeps associated crates in subfolders of the same git repo, but doing that now seems like it'd be painful for git blame (and code review), so a new repo like servo/string-cache-codegen might be better?

i.e. moving the entire existing crate into a subdir called string_cache seems like it'd pollute the git history? Or are you suggesting keeping string-cache at the top level, creating a string_cache_codegen subfolder and adding that folder to the exclude of the string_cache Cargo.toml?

SimonSapin · 2016-01-29T15:32:46Z

I don't understand the concern with git history. Theses crates are very much related, changes in one are relevant to the other. On the other hand, coordinating PRs that depend on each other across repositories tends to be a pain.

I don't care too much about the directory structure within one repository.

aidanhs · 2016-01-29T15:42:38Z

I don't understand the concern with git history.

As in, doing git mv * string-cache/ creates a big diff which can be difficult to navigate around using the git cli (e.g. rust-lang/rust@024aa9a). However, I realised Cargo.toml has exclude so that's actually not necessary.

I've made the change adding string-cache-codegen as a subfolder.

aidanhs · 2016-02-28T03:59:45Z

Rebased to fix conflicts.

Both crates are within one repo and Cargo.toml at the root level contains exclude = ["string-cache-codegen/**/*"].

I've updated the top PR comment since it was a bit out-of-date.

frewsxcv · 2016-02-28T04:14:05Z

Cargo.toml

-phf_generator = "0.7.4"
-phf_shared = "0.7.4"
+[build-dependencies.string_cache_codegen]
+version = "0.2.11"


You probably need to specify path = "string-cache-codegen" here

Yes, that line is noted in the first comment of this PR as part of the instructions for trying it out.

However, I believe that won't work for downloads from crates.io because of exclude = ["string-cache-codegen/**/*"]. I added this exclude line because I think string-cache should consume string-cache-codegen from crates.io (like anything else will need to).

I could

add path = to demonstrates the tests pass or

add path = and remove exclude = for the purposes of this PR, then make a followup PR once string-cache-codegen is published to crates.io undoing this

However, I believe that won't work for downloads from crates.io because of exclude = ["string-cache-codegen/**/*"]. I added this exclude line because I think string-cache should consume string-cache-codegen from crates.io (like anything else will need to).

I'm pretty sure you just need a path dependency, with no exclude at all, just like phf does.

It turns out I didn't understand what path dependencies do (rust-lang/cargo#2625 - when pulling from crates.io, path = are ignored).

I've added path (so tests will work) and left in the exclude (there is no point in users getting the string-cache-codegen folder because Cargo will ignore it).

aidanhs · 2016-04-28T00:28:04Z

Need to wait for phf 0.7.15 to pick up rust-phf/rust-phf#81

aidanhs · 2016-05-30T20:34:29Z

Alright, I'm happy with the state of this PR now the latest phf has been released - could you review? I've been using this locally for a while and it's worked fine, and I've just successfully done a servo build which seems ok (I had to override the path to phf in addition to string-cache because I couldn't seem to get mach update-cargo -p phf to alter the Cargo.lock).

SimonSapin · 2016-05-30T21:18:45Z

Did you not have to change html5ever to do a Servo build? If you can push that Servo branch somewhere that would help too. Thanks!

aidanhs · 2016-05-30T21:58:33Z

Per message on irc: it was a build from master of servo, no changes required aside from a .cargo/config to override the phf and string-cache deps.

(when servo takes this update it'll need to update phf to version 0.7.15, but I couldn't figure out how to get mach to do that, per my previous comment)

bors-servo · 2016-07-22T14:20:05Z

☔ The latest upstream changes (presumably #165) made this pull request unmergeable. Please resolve the merge conflicts.

aidanhs · 2016-07-25T23:36:03Z

Rebased.

bors-servo · 2016-08-03T08:30:24Z

☔ The latest upstream changes (presumably #166) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-08-26T13:32:52Z

☔ The latest upstream changes (presumably #167) made this pull request unmergeable. Please resolve the merge conflicts.

aidanhs · 2016-09-16T03:27:57Z

Rebased.

nox · 2016-09-22T13:03:47Z

As in, doing git mv * string-cache/ creates a big diff which can be difficult to navigate around using the git cli (e.g. rust-lang/rust@024aa9a). However, I realised Cargo.toml has exclude so that's actually not necessary.

I don't understand this, what's hard to read in that diff? Git handles moves.

aidanhs · 2016-09-24T12:50:45Z

I don't understand this, what's hard to read in that diff? Git handles moves.

Github has improved their handling of renames in the time since I wrote that (I'm aware the CLI was always fine). It's irrelevant now though since I like the current arrangement and it works fine.

nox · 2016-09-24T12:51:36Z

I don't really like when there are crates in subdirectories of another. I would rather we move string_cache to its own subdirectory too.

aidanhs · 2016-09-24T13:00:49Z

To offer a counterargument: the codegen crate is pretty thin, I suspect the majority of people will be interested in the code string-cache itself so it kinda makes it easier having it at the top (the readme can contain details of both crates, I just haven't done it yet).

That said, I don't have strong feelings so if that's a consensus among crate maintainers/should be considered a review comment, I'll change it.

aidanhs · 2016-10-13T23:58:07Z

So, how's the review coming along? :) I've now finished v0.1 of my project but I can't publish to crates.io with git repositories in the sources.

If you think it'll take another few months to get around to reviewing etc I'll just vendor string-cache for a while - no big deal. Just want to get some idea of where this is at.

This new dependency is temporary. With #136 it’ll go into html5ever.

SimonSapin · 2016-10-27T08:15:06Z

I apologize for procrastinating for so long the review of this PR.

There was a number of things that I wanted to change / tweak, so rather than micro-manage a review to make you do them I ended up making a new PR at #178. Few actual lines of code are in common, but the basic idea is the same as in this PR and some of the commits are attributed to you.

Of the changes, a big one is not trying to preserve compatibility. One of the goals is to remove the big list of static strings from this crate, so it’s gonna be a breaking change.

dynamic_hash defeats the point of get_index_or_hash returning a hash. Which was, in case the phf lookup fails (the input is not a static atom), re-using that same hash for insertion in the dynamic atom table and avoid hashing the string twice. See #103.

The Borrowed* types are not used anymore in selectors and Servo. Since we’re making breaking changes anyway, this is an opportunity to remove them.

I made the codegen slightly more ergonomic, and support having the generated code included in a module (not at the crate’s root) by taking a path rather than just an identifier.

Closing in favor of #178, thank you for your work!

@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 force-pushed the aphs-static-atom-codegen branch from 82be23e to ff4d798 Compare January 29, 2016 15:46

aidanhs force-pushed the aphs-static-atom-codegen branch 3 times, most recently from 6f65787 to 36184f5 Compare February 28, 2016 03:56

frewsxcv reviewed Feb 28, 2016
View reviewed changes

aidanhs force-pushed the aphs-static-atom-codegen branch from 36184f5 to 4d2419d Compare February 28, 2016 14:06

aidanhs mentioned this pull request Feb 28, 2016

Allow specifying a module path to the phf crate for codegen rust-phf/rust-phf#81

Merged

SimonSapin mentioned this pull request Mar 4, 2016

We should start caching strings, instead of copying them all around fxbox/deprecated-taxonomy#11

Closed

aidanhs force-pushed the aphs-static-atom-codegen branch 3 times, most recently from 33f805a to 354b7f6 Compare April 28, 2016 00:04

aidanhs force-pushed the aphs-static-atom-codegen branch 3 times, most recently from c78e9e3 to f051456 Compare May 30, 2016 19:41

aidanhs mentioned this pull request Jun 1, 2016

Minor optimizer simplification opportunity emscripten-core/emscripten#4367

Closed

SimonSapin mentioned this pull request Jul 13, 2016

Move to cargo workspaces and single toplevel Cargo.lock servo/servo#12391

Closed

aidanhs force-pushed the aphs-static-atom-codegen branch from f051456 to 642c066 Compare July 25, 2016 22:50

aidanhs force-pushed the aphs-static-atom-codegen branch from 642c066 to 1e96906 Compare August 3, 2016 13:02

aidanhs force-pushed the aphs-static-atom-codegen branch from 1e96906 to bd50cea Compare August 3, 2016 13:26

aidanhs mentioned this pull request Aug 3, 2016

Types have known heap sizes, not idents servo/heapsize#60

Merged

aidanhs force-pushed the aphs-static-atom-codegen branch from bd50cea to 359127a Compare August 3, 2016 15:51

Add and use new string-cache-codegen crate

125e9dd

aidanhs force-pushed the aphs-static-atom-codegen branch from 359127a to 125e9dd Compare September 16, 2016 03:18

SimonSapin added a commit that referenced this pull request Oct 26, 2016

Implement selectors_bloom::BloomHash for Atom and Namespace.

aaa0b33

This new dependency is temporary. With #136 it’ll go into html5ever.

This was referenced Oct 26, 2016

Implement selectors_bloom::BloomHash for Atom and Namespace. #177

Closed

Make Atom generic over the set of static strings #178

Merged

SimonSapin closed this Oct 27, 2016

Use new string_cache_codegen crate for static atoms #136

Use new string_cache_codegen crate for static atoms #136

Uh oh!

Conversation

aidanhs commented Jan 29, 2016

Uh oh!

SimonSapin commented Jan 29, 2016

Uh oh!

aidanhs commented Jan 29, 2016

Uh oh!

SimonSapin commented Jan 29, 2016

Uh oh!

aidanhs commented Jan 29, 2016

Uh oh!

aidanhs commented Feb 28, 2016

Uh oh!

frewsxcv Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

aidanhs Feb 28, 2016

Choose a reason for hiding this comment

Uh oh!

nox Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidanhs commented Apr 28, 2016

Uh oh!

aidanhs commented May 30, 2016

Uh oh!

SimonSapin commented May 30, 2016

Uh oh!

aidanhs commented May 30, 2016

Uh oh!

bors-servo commented Jul 22, 2016

Uh oh!

aidanhs commented Jul 25, 2016

Uh oh!

bors-servo commented Aug 3, 2016

Uh oh!

bors-servo commented Aug 26, 2016

Uh oh!

aidanhs commented Sep 16, 2016

Uh oh!

nox commented Sep 22, 2016

Uh oh!

aidanhs commented Sep 24, 2016

Uh oh!

nox commented Sep 24, 2016

Uh oh!

aidanhs commented Sep 24, 2016

Uh oh!

aidanhs commented Oct 13, 2016

Uh oh!

SimonSapin commented Oct 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aidanhs Apr 27, 2016 •

edited by larsbergstrom

Loading