Skip to content

/rust-bindgen

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

No more syntex #940

Merged
merged 12 commits into from Sep 7, 2017

Conversation

Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@fitzgen @highfive @bors-servo @emilio @glyn
@fitzgen
Copy link
Member

fitzgen commented Aug 31, 2017

There are a few commits in this PR, but the big one is the commit that goes syntex -> quote for code generation. I've included a copy of its commit message below.

I tried to verify that it works with the stylo build still, but there were some issues, and then I checked with master, and that wasn't working either. So now I'm C-Reducing the failures on master and figured that this is at least ready for feedback in the meantime.

r? @emilio

The syntex crate is unmaintained. It is slow to build, and additionally it requires that we pre-process src/codegen/mod.rs before we build the bindgen crate.

The quote crate provides similar quasi-quoting functionality, is maintained, and builds faster. It doesn't have a typed API or builders, however; it only deals with tokens.

Before this commit:

$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs

After this commit:

$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs

Build time is cut in half! But what about run time?

Before this commit:

Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }

After this commit:

Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }

So it appears to be about 20ms slower at generating Stylo bindings, but I suspect this is well within the noise.

Finally, this also lets us remove that nasty mem::transmute inside bindgen::ir::BindgenContext::gen that was used for the old syntex context. Now BindgenContext doesn't have a lifetime parameter either. This should make it easier to revisit doing our analyses in parallel with rayon, since that context was one of the things that made it hard for BindgenContext to implement Sync.

Fixes #925

@highfive highfive assigned emilio Aug 31, 2017

@highfive highfive added the S-awaiting-review label Aug 31, 2017

@highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

highfive commented Aug 31, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@fitzgen fitzgen force-pushed the fitzgen:no-syntex branch 2 times, most recently from d5f4753 to d130bf6 Sep 1, 2017

@emilio

emilio reviewed Sep 4, 2017
View changes

Copy link
Collaborator

emilio left a comment

So, looks pretty reasonable. If I understand it well, we're running rustfmt on the bindings now, and we need to have the latest rustfmt-nightly installed locally to run tests so that they pass on CI.

Is that right? If so, I think we should document that.
src/codegen/helpers.rs Outdated
pub fn link_name(name: &str) -> quote::Tokens {
// This is ridiculous, but rustfmt doesn't seem to be formatting
// `link_name` attributes very well, so we jump through these formatting
// hoops to make it work.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Sep 4, 2017

Collaborator

Can you file an issue upstream and reference it here?

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Sep 5, 2017

Author Member

I've filed a handful of bugs upstream, and they've all been fixed now, so this comment might actually be out of date. I'll double check.
src/codegen/helpers.rs
.lit()
.byte_str(string),
)
let b = quote::ByteStr(&string);

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Sep 4, 2017

Collaborator

ooh, nice

@glyn glyn referenced this pull request Sep 4, 2017

Closed

bitfield methods not generated #816

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 5, 2017

If I understand it well, we're running rustfmt on the bindings now, and we need to have the latest rustfmt-nightly installed locally to run tests so that they pass on CI.

Is that right? If so, I think we should document that.

Yes, it will be automatically installed by the Once callback in the rustfmt function in the tests file.

I'll document it in CONTRIBUTING.md as well.
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 5, 2017

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

@highfive highfive added the S-needs-rebase label Sep 5, 2017

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 5, 2017

  • Added note about rustfmt in CONTRIBUTING.md
  • Removed the funky formatting for link_name -- new rustfmt has fixed the bug we were running into

re-r? @emilio
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 5, 2017

Oops, just saw I need to rebase now.

@fitzgen fitzgen force-pushed the fitzgen:no-syntex branch from 43806e3 to 21f8bf8 Sep 5, 2017

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 5, 2017

Ok, rebased.
@emilio

emilio reviewed Sep 6, 2017
View changes

Copy link
Collaborator

emilio left a comment

Looks ok, but I don't feel like updating nightly / rustfmt on behalf of the user is right... :/
CONTRIBUTING.md

```
$ rustup update nightly
$ rustup run nightly cargo install -f rustfmt-nightly

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Sep 6, 2017

Collaborator

What happens when rustfmt changes the default formatting? All our CI breaks until we update them? That seems not great :(

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Sep 6, 2017

Author Member

Nope, because both the generated bindings and the expectations are run through rustfmt before being compared.
tests/tests.rs Outdated
// Because `rustfmt` needs to match its exact nightly version, we update
// both at the same time.

let status = process::Command::new("rustup")

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Sep 6, 2017

Collaborator

It definitely feels wrong to update the nightly in behalf of the user.

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Sep 6, 2017

Author Member

Unfortunately, this is the only way for the latest rustfmt to work :-/
tests/tests.rs Outdated
assert!(status.success(), "should run `rustup update nightly` OK");

let status = process::Command::new("rustup")
.args(&["run", "nightly", "cargo", "install", "-f", "rustfmt-nightly"])

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Sep 6, 2017

Collaborator

Ditto...
@emilio

This comment has been minimized.

Sign in to view
Copy link
Collaborator

emilio commented Sep 6, 2017

(Of course this is just for contributors, but still)
@emilio

This comment has been minimized.

Sign in to view
Copy link
Collaborator

emilio commented Sep 6, 2017

Oh, also @glyn reported when building on top of this branch that he got unexpected test expectation changes, so we should probably get that sorted out.
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 6, 2017

@glyn, do you have the latest rustfmt installed? Can you give more info about the expectation changes you were seeing?
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 6, 2017

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

@highfive highfive added the S-needs-rebase label Sep 6, 2017

@glyn

This comment has been minimized.

Sign in to view
Copy link
Contributor

glyn commented Sep 7, 2017

@fitzgen Yes, I believe I was using the latest rustfmt. Details in this gist.
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 7, 2017

@fitzgen Yes, I believe I was using the latest rustfmt. Details in this gist.

Aha! You need to use rustfmt-nightly, which is where rustfmt development has moved to. The plain rustfmt crate is unmaintained now.

fitzgen added some commits Aug 23, 2017

@fitzgen
Run `rustfmt` on the test actual output and expectations
7346811
@fitzgen
Add the TypeKind name to the IR graphviz output
66743be
@fitzgen
Use `quote` instead of `syntex` for Rust code generation 
The `syntex` crate is unmaintained. It is slow to build, and additionally it
requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen`
crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained,
and builds faster. It doesn't have a typed API or builders, however; it only
deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I
suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside
`bindgen::ir::BindgenContext::gen` that was used for the old `syntex`
context. Now `BindgenContext` doesn't have a lifetime parameter either. This
should make it easier to revisit doing our analyses in parallel with `rayon`,
since that context was one of the things that made it hard for `BindgenContext`
to implement `Sync`.

Fixes #925
61743aa
@fitzgen
Print time phases for the stylo sanity test
4f0aa3e
@fitzgen
Fix units and formatting of time phases 
Was previously printing seconds, but claiming it was milliseconds.
6e4383a
@fitzgen
We can afford the exra cycles to use double precision floats here 
;)
a93d0fb
@fitzgen
Add a timer for rustfmt'ing the generated bindings
17f13b3
@fitzgen
Explicitly install the latest rustfmt in CI
efdb1c5

@fitzgen fitzgen force-pushed the fitzgen:no-syntex branch from 21f8bf8 to 46f74c1 Sep 7, 2017

@emilio

emilio approved these changes Sep 7, 2017
View changes

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member Author

fitzgen commented Sep 7, 2017

@bors-servo r=emilio
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 7, 2017

📌 Commit 46f74c1 has been approved by emilio

@highfive highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 7, 2017

@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 7, 2017

⌛️ Testing commit 46f74c1 with merge b13f076...

bors-servo added a commit that referenced this pull request Sep 7, 2017

@bors-servo
Auto merge of #940 - fitzgen:no-syntex, r=emilio 
No more syntex

There are a few commits in this PR, but the big one is the commit that goes `syntex` -> `quote` for code generation. I've included a copy of its commit message below.

I tried to verify that it works with the stylo build still, but there were some issues, and then I checked with master, and that wasn't working either. So now I'm C-Reducing the failures on master and figured that this is at least ready for feedback in the meantime.

r? @emilio

----------------------------

The `syntex` crate is unmaintained. It is slow to build, and additionally it requires that we pre-process `src/codegen/mod.rs` before we build the `bindgen` crate.

The `quote` crate provides similar quasi-quoting functionality, is maintained, and builds faster. It doesn't have a typed API or builders, however; it only deals with tokens.

Before this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 98.75 secs
```

After this commit:

```
$ cargo clean; cargo build
<snip>
    Finished dev [unoptimized + debuginfo] target(s) in 46.26 secs
```

Build time is cut in half! But what about run time?

Before this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 521105668 }
```

After this commit:

```
Generated Stylo bindings in: Duration { secs: 3, nanos: 548797242 }
```

So it appears to be about 20ms slower at generating Stylo bindings, but I suspect this is well within the noise.

Finally, this also lets us remove that nasty `mem::transmute` inside `bindgen::ir::BindgenContext::gen` that was used for the old `syntex` context. Now `BindgenContext` doesn't have a lifetime parameter either. This should make it easier to revisit doing our analyses in parallel with `rayon`, since that context was one of the things that made it hard for `BindgenContext` to implement `Sync`.

Fixes #925
Loading status checks…
b13f076
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Sep 7, 2017

☀️ Test successful - status-travis
Approved by: emilio
Pushing b13f076 to master...

@bors-servo bors-servo merged commit 46f74c1 into rust-lang:master Sep 7, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@highfive highfive removed the S-awaiting-merge label Sep 7, 2017

@glyn

This comment has been minimized.

Sign in to view
Copy link
Contributor

glyn commented Sep 8, 2017

@fitzgen rm ~/.cargo/bin/rustfmt fixed the problem, thanks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.