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

Replaced linear token counting macros with optimized implementation #59600

Merged
merged 1 commit into from Jun 10, 2019

Conversation

Projects
None yet
8 participants
@tobia
Copy link
Contributor

commented Apr 1, 2019

There are currently two distinct token-counting macros in the source. Both implement the trivial algorithm, with linear complexity. They may or may not be adequate for their use case, but considering that other people are probably going to copy and paste them whenever they need a token-counting macro, I replaced them with an optimized implementation with logarithmic complexity.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:02f2ff0d:start=1554110687908288597,finish=1554110688767636698,duration=859348101
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:05:14]    Compiling rustc_apfloat v0.0.0 (/checkout/src/librustc_apfloat)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:735:32
[00:05:14]     |
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14]     | |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14]     | |_- in this expansion of `count!`
[00:05:14] 738 | 
[00:05:14] 739 | / macro_rules! tuple {
[00:05:14] 740 | |     () => ();
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | |_- in this expansion of `tuple!`
[00:05:14]     | |_- in this expansion of `tuple!`
[00:05:14] 771 | 
[00:05:14] 772 |   tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     |   ------------------------------------------------------------ in this macro invocation
[00:05:14]     = note: `-D unused-parens` implied by `-D warnings`
[00:05:14] 
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:736:32
[00:05:14]    --> src/libserialize/serialize.rs:736:32
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:735:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14]     | |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:736:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:735:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14]     | |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:736:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:735:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14]     | |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:736:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:735:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14]     | |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:736:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:14] error: unnecessary parentheses around assigned value
[00:05:14]    --> src/libserialize/serialize.rs:735:32
[00:05:14]     |
[00:05:14]     |
[00:05:14] 722 | / macro_rules! peel {
[00:05:14] 723 | |     ($name:ident, $($other:ident,)*) => (tuple! { $($other,)* })
[00:05:14] 724 | | }
[00:05:14] 724 | | }
[00:05:14]     | |_- in this expansion of `peel!` (#2)
[00:05:14] 732 | / macro_rules! count {
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 733 | |     ()                     => (0usize);
[00:05:14] 734 | |     ($one:tt)              => (1usize);
[00:05:14] 735 | |     ($($pairs:tt $_p:tt)*) => ((count!($($pairs)*) << 1usize));
[00:05:14]     | |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
[00:05:14] 736 | |     ($odd:tt $($rest:tt)*) => ((count!($($rest)*) | 1usize));
[00:05:14] 737 | | }
[00:05:14]     | |_- in this expansion of `count!` (#4)
[00:05:14] 739 |   macro_rules! tuple {
[00:05:14]     |  _-
[00:05:14]     | |_|
[00:05:14]     | |
[00:05:14]     | |
[00:05:14] 740 | |     () => ();
[00:05:14] 741 | |     ( $($name:ident,)+ ) => (
[00:05:14] 742 | |         impl<$($name:Decodable),*> Decodable for ($($name,)*) {
[00:05:14] ...   |
[00:05:14] 745 | |                 let len: usize = count!($($name)*);
[00:05:14] ...   |
[00:05:14] ...   |
[00:05:14] 768 | |         peel! { $($name,)* }
[00:05:14] 769 | |     )
[00:05:14] 770 | | }
[00:05:14]     | | -
[00:05:14]     | |_|
[00:05:14]     | |_|
[00:05:14]     | |_in this expansion of `tuple!` (#1)
[00:05:14]     |   in this expansion of `tuple!` (#3)
[00:05:14] 771 | 
[00:05:14] 772 | | tuple! { T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, }
[00:05:14]     | | ------------------------------------------------------------ in this macro invocation (#1)
[00:05:16] error: aborting due to 11 previous errors
[00:05:16] 
[00:05:16] error: Could not compile `serialize`.
[00:05:16] warning: build failed, waiting for other jobs to finish...

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Should we deduplicate one of these macros?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

I don't know if the size of the final generated code here matters in practice. The previous code being generated looks quite trivial to optimize; but the logarithmic variant probably also optimizes well.

Out of curiosity, did you attempt to measure the expansion time itself of the old and new versions on large inputs?

Update: I don't want to give the wrong impression: I'm not objecting to this PR. I just was a little surprised to see it.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

(also, in case you hadn't noticed, travis is complaining about unneeded parentheses in the macro definitions here...)

@tobia tobia force-pushed the tobia:master branch from 559d330 to a4a07e0 Apr 12, 2019

@tobia

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Hello. I've removed the extra parentheses, let's see if the warnings are gone.

To me it seemed obvious that a linear counter was a weak solution, that could cause trouble sooner or later. Be it for the time taken to count things, for the memory used by macro recursion, or for Rust's default recursion limit. Considering that a token counter could conceivably be used to implement initializers like vec![...] (even though the standard ones don't use it) having someone apply such an initializer to a large number of elements, only to get a macro expansion error, is in the realm of possibility.

I agree that it should be deduplicated in a standard place. But I don't know Rust's sources well enough to suggest the correct place. Also, there may be other instances of that linear counting algorithm elsewhere in the source tree, that my grep-fu didn't turn up.

Anyways, I did an accurate measurement, using this source code. I did the various measurements by hand, changing the number of tokens in the macro call, commenting / decommenting the two macros, and running each test several times in a Bash loop, with rustc test_count.rs && ./test_count, in order to get some precision when I computed the average. For example, the smaller tests would return either 0 or 1 ticks, so I ran 100 of them and computed the average. I also did a measurement with no count!() macro call and subtracted that baseline from all other results. Ticks per second can be obtained with the getconf CLK_TCK command. Results:

Tokens New macro Old macro
10 0.1ms 0.1ms
100 0.2ms (*) 3.1ms
1,000 2.9ms (*) 232.6ms
10,000 8.9ms ERROR
100,000 90.3ms ERROR

(*) Rust's default recursion limit does not allow the old linear macro to run on more than 61 tokens, so for the test's sake I artificially increased it with ![recursion_limit="..."]. Still, beyond 1,000 tokens even that was not enough: the compiler crashed with Fatal runtime error: stack overflow.

Also, in case anybody is curious, this is the expansion for 10,000 tokens:

(((((1 << 1 << 1 << 1 | 1) << 1 | 1) << 1 | 1) << 1 << 1 << 1 << 1 | 1) << 1 << 1 << 1 << 1)

Compare that to summing 1 to itself 10,000 times, in a non-tail recursive call, needing 10,000 stack frames.

@Centril

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@rust-highfive rust-highfive assigned pnkfelix and unassigned KodrAus Apr 28, 2019

@panaman67

This comment has been minimized.

Copy link

commented May 18, 2019

Whats the status on this?

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Ping from triage @pnkfelix

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

📌 Commit a4a07e0 has been approved by pnkfelix

@lzutao

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

This PR may affect parsing performance. Do it need to be rollup?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #61506
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

📌 Commit a4a07e0 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

I'm having some issues interacting with github at the moment (thus the duplicate approvals). I don't care one way or another whether this is rolled up. But I don't expect there to be much impact on performance from it one way or another (apart from the case of microbenchmarks like the ones given above).

Centril added a commit to Centril/rust that referenced this pull request Jun 10, 2019

Rollup merge of rust-lang#59600 - tobia:master, r=pnkfelix
Replaced linear token counting macros with optimized implementation

There are currently two distinct token-counting macros in the source. Both implement the trivial algorithm, with linear complexity. They may or may not be adequate for their use case, but considering that other people are probably going to copy and paste them whenever they need a token-counting macro, I replaced them with an optimized implementation with logarithmic complexity.

Centril added a commit to Centril/rust that referenced this pull request Jun 10, 2019

Rollup merge of rust-lang#59600 - tobia:master, r=pnkfelix
Replaced linear token counting macros with optimized implementation

There are currently two distinct token-counting macros in the source. Both implement the trivial algorithm, with linear complexity. They may or may not be adequate for their use case, but considering that other people are probably going to copy and paste them whenever they need a token-counting macro, I replaced them with an optimized implementation with logarithmic complexity.

Centril added a commit to Centril/rust that referenced this pull request Jun 10, 2019

Rollup merge of rust-lang#59600 - tobia:master, r=pnkfelix
Replaced linear token counting macros with optimized implementation

There are currently two distinct token-counting macros in the source. Both implement the trivial algorithm, with linear complexity. They may or may not be adequate for their use case, but considering that other people are probably going to copy and paste them whenever they need a token-counting macro, I replaced them with an optimized implementation with logarithmic complexity.

bors added a commit that referenced this pull request Jun 10, 2019

Auto merge of #61716 - Centril:rollup-nxwf5ol, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #59600 (Replaced linear token counting macros with optimized implementation)
 - #61501 (get rid of real_intrinsics module)
 - #61570 (Fix issues with const argument inference)
 - #61683 (Haiku: the maximum stack size is 16 MB)
 - #61697 (submodules: update clippy from 71be6f62 to c0dbd34b)

Failed merges:

r? @ghost

@bors bors merged commit a4a07e0 into rust-lang:master Jun 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
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.