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

Draft: RFC configuration in const context & HTTP-like protocol parsing support #129

Closed
wants to merge 2 commits into from

Conversation

gootorov
Copy link

I do not expect this PR to be merged as-is (or at all). The main purpose of this is to have a basis for discussion, hear different opinions and suggestions.

Some of the changes here are breaking, I'll leave additional information concerning each commit as a separate comment.

@gootorov
Copy link
Author

gootorov commented Nov 25, 2022

First commit makes the following usage possible:

        const CONFIG: super::ParserConfig = super::ParserConfig::const_default()
            .allow_spaces_after_header_name_in_responses(true)
            .allow_multiple_spaces_in_request_line_delimiters(true);

which is otherwise rejected due to mutable references usage in const context.

For my use case in particular, this is just ergonomics. I know quite specifically the configuration I need, and it won't be changed. Of course, there are workarounds, like using lazy_static/once_cell, or simply inlining usual instantiation, so it's not exactly a big problem.

One thing though that I hoped to see is branch elimination. To be honest, I do not know enough about const propagation in rustc, but I hoped maybe if the compiler knew all options, it would be able to inline new branchless functions. However, looking at the assembly, it seems there are no changes at all. Maybe this will come in the future, or maybe I am asking for too much :)

Do you think something like this is worth a breaking change?

@gootorov
Copy link
Author

Second commit is another stab at supporting HTTP-like protocols.

The main idea is that httparse can implement several parsers for the request/response-line and the user can choose these parsers through ParserConfig.

The commit here is does not implement this idea fully. I used SIP as the example, the only difference there is in version parsing in the response, but I suppose there could be other protocols that could differ slightly more.
In which case, this could be refactored further to contain two function pointers, for request-line and response-line parsing for each supported respectively. Similarly to SIP, there have been requests for ICAP support in #128 and RTSP in #67.

Although as much as I would love to see this supported, this seems just a little bit out of project's scope. What do you think?

Regarding performance, this is what I got before and after this commit:

req/req                 time:   [327.43 ns 328.13 ns 328.83 ns]                    
                        thrpt:  [2.0392 GiB/s 2.0435 GiB/s 2.0479 GiB/s]
                 change:
                        time:   [+1.4046% +2.1175% +2.8697%] (p = 0.00 < 0.05)
                        thrpt:  [-2.7896% -2.0736% -1.3851%]
                        Performance has regressed.

req_short/req_short     time:   [65.513 ns 65.648 ns 65.785 ns]                                 
                        thrpt:  [985.79 MiB/s 987.84 MiB/s 989.87 MiB/s]
                 change:
                        time:   [-5.4697% -4.7538% -4.1018%] (p = 0.00 < 0.05)
                        thrpt:  [+4.2772% +4.9911% +5.7862%]
                        Performance has improved.

resp/resp               time:   [345.99 ns 346.97 ns 348.03 ns]                      
                        thrpt:  [1.8705 GiB/s 1.8763 GiB/s 1.8815 GiB/s]
                 change:
                        time:   [+4.8074% +5.5219% +6.3035%] (p = 0.00 < 0.05)
                        thrpt:  [-5.9297% -5.2330% -4.5869%]
                        Performance has regressed.

resp_short/resp_short   time:   [70.511 ns 70.656 ns 70.797 ns]                                   
                        thrpt:  [1.1971 GiB/s 1.1995 GiB/s 1.2019 GiB/s]
                 change:
                        time:   [-2.7264% -2.0204% -1.3541%] (p = 0.00 < 0.05)
                        thrpt:  [+1.3726% +2.0621% +2.8028%]
                        Performance has improved.

Which is a weird result. I am benchmarking this on a laptop though, so there might actually be no meaningful difference and this is just a fluke.

/// Sets first-line parsing to SIP
pub const fn set_sip_protocol_parser(mut self) -> Self {
self.version_parser = SIP_VERSION_PARSER;
self
Copy link
Author

Choose a reason for hiding this comment

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

I would love to see this as something like the following:

pub const fn set_protocol_parser(mut self, protocol: Protocol) -> Self {
    self.version_parser = match protocol {
        Protocol::Http => HTTP_VERSION_PARSER,
        Protocol::Sip => SIP_VERSION_PARSER,
    };
    self
}

However, that gives the following compilation error:

error[E0658]: mutable references are not allowed in constant functions
   --> src/lib.rs:287:31
    |
287 |           self.version_parser = match protocol {
    |  _______________________________^
288 | |             Protocol::Http => HTTP_VERSION_PARSER,
289 | |             Protocol::Sip => SIP_VERSION_PARSER,
290 | |         };
    | |_________^
    |
    = note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information

which is weird :|

Copy link

Choose a reason for hiding this comment

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

@gootorov Similar code works fine here. What is the difference?

Copy link

Choose a reason for hiding this comment

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

I mean, is it only the attribute type, or the Protocol enum too?

allow_multiple_spaces_in_request_line_delimiters: false,
allow_multiple_spaces_in_response_status_delimiters: false,
ignore_invalid_headers_in_responses: false,
version_parser: HTTP_VERSION_PARSER,
Copy link
Author

Choose a reason for hiding this comment

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

For the same reason, this also errors, but putting the function pointer in const first makes it go away. Interesting.

@gootorov gootorov closed this Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants