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.

Sign up for GitHub

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

Jump to bottom

No more syntex for serde_derive #548

Merged
merged 19 commits into from
Sep 28, 2016
Merged

No more syntex for serde_derive #548

merged 19 commits into from
Sep 28, 2016

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Sep 12, 2016 •
edited
Loading

Follow-up tasks:

@dtolnay dtolnay added the wip label Sep 12, 2016
@dtolnay
Copy link
Member Author

dtolnay commented Sep 12, 2016

Running cargo test in serde_derive now passes! It didn't before due to rust-lang/rust#36211.

@dtolnay
Copy link
Member Author

dtolnay commented Sep 12, 2016

For some reason serde_codegen by itself builds three times as fast this way, which I didn't expect. I guess quasi_macros is slow?

$ git checkout master
$ cargo build --no-default-features --features unstable
$ cargo clean -p serde_codegen
$ cargo build --no-default-features --features unstable  # 12 SECONDS
$ git checkout syn
$ cargo build --no-default-features --features with-syn
$ cargo clean -p serde_codegen
$ cargo build --no-default-features --features with-syn  # 4 SECONDS

@oli-obk
Copy link
Member

oli-obk commented Sep 12, 2016

I guess quasi_macros is slow?

fewer generics?

This was referenced Sep 13, 2016
Closed
Closed
@dtolnay
Copy link
Member Author

dtolnay commented Sep 13, 2016

I did some testing with rusoto in rusoto/rusoto#364 (comment) to see how performance of this PR compares with the current serde_macros (runtime performance, after they have been compiled). They come out about even which is great.

@dtolnay
Merge branch origin/master into origin/syn
51f9557 
Conflicts:
    serde_codegen/Cargo.toml
    serde_codegen_internals/Cargo.toml
    serde_derive/Cargo.toml
@dtolnay
Bump syn dependency
1eca776
@dtolnay
Merge branch origin/master into origin/syn
6cbf0d3 
Conflicts:
    serde_codegen/src/ser.rs
@dtolnay
Fix conflicts with the __serde_state commit
8e865f6
@dtolnay dtolnay mentioned this pull request Sep 27, 2016
Closed
@dtolnay dtolnay mentioned this pull request Sep 27, 2016
Merged
7 tasks
@dtolnay
Copy link
Member Author

dtolnay commented Sep 27, 2016

There is a lot of work left here but let's take a moment to celebrate 9a86e68 the first syntex-free green build.

@dtolnay dtolnay removed the wip label Sep 27, 2016
@dtolnay
Copy link
Member Author

dtolnay commented Sep 27, 2016 •
edited
Loading

@oli-obk @erickt this is ready for review. There were a few places that I uncovered strange behavior in the old codegen (just unusually placed semicolons / parentheses / braces) that I went out of my way to preserve. I left comments and I can clean that up in a separate PR. I confirmed that this PR expands the test suite byte-for-byte identical to master. I filed #562 to follow up.

I filed #561 as another follow-up task. I don't think that needs to block this PR.

@dtolnay dtolnay mentioned this pull request Sep 27, 2016
Closed
This was referenced Sep 27, 2016
Closed
Closed
Closed
@alexcrichton
Copy link
Contributor

Awesome progress @dtolnay!

oli-obk
oli-obk approved these changes Sep 28, 2016
View reviewed changes
Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

This is totally great. I have no complaints that should be seen as blockers. Let's do this!

serde_codegen/src/de.rs
Some(quote!(::<#(ty_param_idents),*>))
};

let phantom_exprs = (0 .. num_phantoms).map(|_| quote!(::std::marker::PhantomData));
Copy link
Member

Choose a reason for hiding this comment

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

std::iter::repeat(quote!(::std::marker::PhantomData)).take(num_phantoms) runs the expanded quote code only once. Not really relevant though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

serde_codegen/src/de.rs
struct __Visitor #generics ( #(phantom_types),* ) #where_clause;
},
quote!(__Visitor <#(all_params),*> ),
quote!(__Visitor #ty_param_idents ( #(phantom_exprs),* )),
Copy link
Member

Choose a reason for hiding this comment

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

❤️ this expansion magic

serde_codegen/src/lib.rs
extern crate aster;
extern crate quasi;
// The `quote!` macro requires deep recursion.
#![recursion_limit = "192"]
Copy link
Member

Choose a reason for hiding this comment

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

I'm through with serde_codegen. Big 👍 on the quote! call readability.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I love how've much simpler everything looks.

serde_codegen_internals/src/ctxt.rs
use std::cell::RefCell;

#[derive(Default)]
pub struct Ctxt {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very happy with the Ctxt structure and it's usage. I'd much rather have regular Result type error reporting, but we can leave that to a future refactoring. Maybe the code gets horribly cluttered with Result

Copy link
Member Author

Choose a reason for hiding this comment

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

I filed #563 to follow up in a future refactoring.

erickt
erickt approved these changes Sep 28, 2016
View reviewed changes
Copy link
Member

@erickt erickt left a comment

Choose a reason for hiding this comment

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

This wasn't amazing. Great work.

serde_codegen/src/lib.rs
extern crate aster;
extern crate quasi;
// The `quote!` macro requires deep recursion.
#![recursion_limit = "192"]
Copy link
Member

Choose a reason for hiding this comment

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

I agree. I love how've much simpler everything looks.

serde_codegen/src/ser.rs
$type_ident::$variant_ident(ref __simple_value) => $block
)
quote! {
// The braces are unnecessary but quasi used to put them in. We
Copy link
Member

Choose a reason for hiding this comment

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

If I recall correctly, quote_block required the extra braces, so shouldn't be a problem to remove them later.

@dtolnay
Use iter::repeat to build phantom exprs
9fe1676
@dtolnay
Merge branch origin/master into origin/syn
b0eee50 
Conflicts:
    serde_macros/tests/compile-fail/reject-unknown-attributes.rs
@dtolnay dtolnay merged commit 49d24a1 into master Sep 28, 2016
@dtolnay dtolnay deleted the syn branch September 28, 2016 16:28
@dtolnay dtolnay mentioned this pull request Sep 28, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk approved these changes

@erickt erickt erickt approved these changes

Assignees

@erickt erickt

@oli-obk oli-obk

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dtolnay @oli-obk @alexcrichton @erickt