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

perf: Reduce the amount of code generated for path! #721

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Oct 14, 2020

This does two things to reduce the amount of code path! generates, first it removes warp::any() filter at the start of each path, therefore removing one level of And as well.

Next I modified the macro to accumulate consecutive string literal segments and combining them into one via concat!. Since each segment generates its own static type this significantly reduce the amount of code when this applies (path!("abc" / "123" / "456")). I did have to cahge the behaviour of path() for this to accept / but I can change this combining to just be an implementation detail instead.

@Marwes
Copy link
Contributor Author

Marwes commented Dec 8, 2020

Any chance of this getting merged?

@seanmonstar
Copy link
Owner

Thanks for the PR! I'd prefer to keep the requirement that exact not allow a string with a slash. What did you have in mind for "I can change this combining to just be an implementation detail instead"?

@Marwes
Copy link
Contributor Author

Marwes commented Dec 9, 2020

I'd just add a #[doc(hidden)] variant of the function which is used by the macro

@Marwes Marwes force-pushed the simplify_path branch 2 times, most recently from 78dbb98 to 91db345 Compare December 22, 2020 13:35
@Marwes
Copy link
Contributor Author

Marwes commented Dec 22, 2020

Rebased with the fix to path

@Marwes
Copy link
Contributor Author

Marwes commented Jan 12, 2021

Any further issues with this? The assert is restored and I added a hidden __path function which the macro calls into which still allows / in the path.

src/filters/path.rs Outdated Show resolved Hide resolved
@Marwes
Copy link
Contributor Author

Marwes commented Feb 5, 2021

@seanmonstar Fixed and added a test

@jxs jxs added the waiting-on-author awaiting some action (such as code changes) from the PR or issue author. label Jun 5, 2021
@ramn
Copy link

ramn commented Jun 11, 2021

I see a new label waiting-on-author was added.

I don't think this PR is waiting on the Author (@Marwes) but rather it is waiting for the maintainer @seanmonstar to review.

@jxs
Copy link
Collaborator

jxs commented Jun 11, 2021

ah righ sorry, as I saw a question still unresolved I thought it wasn't yet addressed. I will try to review this PR this weekend 👍

@jxs jxs added waiting-on-reviewer Awaiting review from the assignee but also interested parties. and removed waiting-on-author awaiting some action (such as code changes) from the PR or issue author. labels Jun 14, 2021
src/filters/path.rs Outdated Show resolved Hide resolved
@jxs
Copy link
Collaborator

jxs commented Jun 21, 2021

LGTM, only some minor nit :)

@jxs jxs added waiting-on-author awaiting some action (such as code changes) from the PR or issue author. and removed waiting-on-reviewer Awaiting review from the assignee but also interested parties. labels Jul 14, 2021
@jxs jxs requested a review from seanmonstar September 3, 2021 00:22
Markus Westerlind added 6 commits March 30, 2023 13:25
By join static string literals in path! we instantiate less `path` calls
and less `and` calls (since there are less things to join).

This changes the behaviour of `path` to accept `/` if this is not wanted
this could instead be hidden as an implementation detail.
Only used once and it bloats the LLVM IR
@Marwes
Copy link
Contributor Author

Marwes commented Mar 30, 2023

Resurrecting this since I am back on project that uses warp. The PR as is seemed to have been mostly accepted (?) but I rebased it (no conflicts) and added the last commit to make the assertion a bit clearer.

Example llvm-lines/(re)compile time for a crate which uses warp::path! quite heavily.

Before

cargo llvm-lines --lib | head -50
   Compiling warp v0.3.3 (/Users/markus.westerlind/Code/warp)
   Compiling a-server v0.1.0 (/Users/markus.westerlind/Code/a/a-server)
    Finished dev [unoptimized + debuginfo] target(s) in 1m 22s
warning: the following packages contain code that will be rejected by a future version of Rust: buf_redux v0.8.4, multipart v0.18.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 1`
  Lines                  Copies               Function name
  -----                  ------               -------------
  1359048                40649                (TOTAL)
   106985 (7.9%,  7.9%)    521 (1.3%,  1.3%)  <warp::filter::and::State<T,TE,U> as core::future::future::Future>::poll
    33276 (2.4%, 10.3%)    708 (1.7%,  3.0%)  core::pin::Pin<P>::set
    32850 (2.4%, 12.7%)    146 (0.4%,  3.4%)  <warp::filters::path::Exact<P> as warp::filter::FilterBase>::filter::{{closure}}::{{closure}}
    22330 (1.6%, 14.4%)    203 (0.5%,  3.9%)  std::thread::local::LocalKey<T>::try_with
    22041 (1.6%, 16.0%)    521 (1.3%,  5.2%)  warp::filter::and::_::<impl warp::filter::and::State<T,TE,U>>::project
    20394 (1.5%, 17.5%)    124 (0.3%,  5.5%)  <warp::filter::and_then::State<T,F> as core::future::future::Future>::poll
    19579 (1.4%, 18.9%)     38 (0.1%,  5.6%)  <&mut serde_json::de::Deserializer<R> as serde::de::Deserializer>::deserialize_struct
    18563 (1.4%, 20.3%)    524 (1.3%,  6.9%)  <warp::filter::and::And<T,U> as warp::filter::FilterBase>::filter
    15736 (1.2%, 21.5%)   2248 (5.5%, 12.4%)  core::pin::Pin<P>::new_unchecked
    14916 (1.1%, 22.6%)     44 (0.1%, 12.5%)  alloc::raw_vec::RawVec<T,A>::grow_amortized

After

cargo llvm-lines --lib | head -50
   Compiling warp v0.3.3 (/Users/markus.westerlind/Code/warp)
   Compiling a-server v0.1.0 (/Users/markus.westerlind/Code/a/a-server)
^Pl^[    Finished dev [unoptimized + debuginfo] target(s) in 1m 17s
warning: the following packages contain code that will be rejected by a future version of Rust: buf_redux v0.8.4, multipart v0.18.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 1`
  Lines                  Copies               Function name
  -----                  ------               -------------
  1270314                37881                (TOTAL)
    68852 (5.4%,  5.4%)    336 (0.9%,  0.9%)  <warp::filter::and::State<T,TE,U> as core::future::future::Future>::poll
    39285 (3.1%,  8.5%)    135 (0.4%,  1.2%)  <warp::filters::path::Exact<P> as warp::filter::FilterBase>::filter::{{closure}}
    24581 (1.9%, 10.4%)    523 (1.4%,  2.6%)  core::pin::Pin<P>::set
    21120 (1.7%, 12.1%)    192 (0.5%,  3.1%)  std::thread::local::LocalKey<T>::try_with
    20390 (1.6%, 13.7%)    124 (0.3%,  3.5%)  <warp::filter::and_then::State<T,F> as core::future::future::Future>::poll
    19579 (1.5%, 15.3%)     38 (0.1%,  3.6%)  <&mut serde_json::de::Deserializer<R> as serde::de::Deserializer>::deserialize_struct
    16616 (1.3%, 16.6%)    425 (1.1%,  4.7%)  <warp::filter::and::And<T,U> as warp::filter::FilterBase>::filter
    14916 (1.2%, 17.7%)     44 (0.1%,  4.8%)  alloc::raw_vec::RawVec<T,A>::grow_amortized
    14828 (1.2%, 18.9%)    125 (0.3%,  5.1%)  alloc::alloc::box_free

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author awaiting some action (such as code changes) from the PR or issue author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants