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

Refactored header name matching for WASM target #324

Merged
merged 2 commits into from Jul 26, 2019

Conversation

matiu2
Copy link
Contributor

@matiu2 matiu2 commented Jul 1, 2019

Advantages:

  1. Shorter code
  2. More rusty and less macroy
  3. It should work in wasm now untested and make yew users happier
    Local count too large yewstack/yew#478

(Un?)fortunately I also ran rustfmt on it - which changed the layouts for the tables a bit but not hideously.

@carllerche
Copy link
Collaborator

Hi, thanks for the work.

Would it be possible to include benchmarks for parsing header names? Probably could add a bench parsing each one + some common unknown header names. It would be interesting to compare the before and after.

@matiu2
Copy link
Contributor Author

matiu2 commented Jul 3, 2019

I tried to make it work in nightly and beta, but then it broke for rust 1.20.

The existing benches are pretty much the same, for example:

New: test basic::insert_500_custom_headers::header_map ... bench: 35,208 ns/iter (+/- 3,372)

Old: test basic::insert_500_custom_headers::header_map ... bench: 36,283 ns/iter (+/- 573)

I'll some benches just for the name parsing..

@matiu2
Copy link
Contributor Author

matiu2 commented Jul 5, 2019

I don't know how to bench unless it's running rust nightly (cargo bench) - so I had to make it work in nightly to get the numbers. It seems pretty close:

cargo bench header_name

old macro style

test header_name_bad ... bench: 15 ns/iter (+/- 0)
test header_name_easy ... bench: 16 ns/iter (+/- 2)
test header_name_various ... bench: 10,222 ns/iter (+/- 991)

new giant match statement

test header_name_bad ... bench: 20 ns/iter (+/- 1)
test header_name_easy ... bench: 21 ns/iter (+/- 1)
test header_name_various ... bench: 9,909 ns/iter (+/- 1,133)

benches/header_name.rs Outdated Show resolved Hide resolved
@matiu2
Copy link
Contributor Author

matiu2 commented Jul 8, 2019

I've moved the make_all_known_headers out of the iterator. Sorry for not noticing that.

This commit is definitely slowing the code down by about 6% on my laptop :(

New bench mark results

Old code:

test header_name_bad ... bench: 15 ns/iter (+/- 11)
test header_name_easy ... bench: 16 ns/iter (+/- 1)
test header_name_various ... bench: 3,689 ns/iter (+/- 290)

New code:

test header_name_bad ... bench: 20 ns/iter (+/- 0)
test header_name_easy ... bench: 24 ns/iter (+/- 1)
test header_name_various ... bench: 3,921 ns/iter (+/- 395)


The main reason for this commit is so we can use http in wasm without an optimized build.

When debug building, those macros that expand out to ...&& b[0 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1] == b'p'... get put as-is into wasm, causing the wasm implementiation to blow up at run-time trying to add all the ones.

Here's the issue in yew that I'm trying to fix: yewstack/yew#478 (comment)

@matiu2 matiu2 force-pushed the master branch 3 times, most recently from 145c2c5 to a90e78a Compare July 9, 2019 03:58
@matiu2
Copy link
Contributor Author

matiu2 commented Jul 9, 2019

Latest bench mark results

Old macro based code:

$ cargo bench header_name
...
test header_name_bad     ... bench:          14 ns/iter (+/- 0)
test header_name_easy    ... bench:          16 ns/iter (+/- 1)
test header_name_various ... bench:       3,763 ns/iter (+/- 80)

New giant match statement code:

test header_name_bad     ... bench:          25 ns/iter (+/- 1)
test header_name_easy    ... bench:          24 ns/iter (+/- 0)
test header_name_various ... bench:       4,227 ns/iter (+/- 75)

@carllerche
Copy link
Collaborator

We should definitely get this working with wasm. Perhaps there is a way to conditional compile a different fn in wasm?

@matiu2
Copy link
Contributor Author

matiu2 commented Jul 12, 2019

This version uses conditional compilation. It'll only compile the wasm version if there are 'debug_assertions' and the target is 'wasm32'.

It make the code longer, and harder to maintain though, because now if you add a new header, you'll have to add it in two places.

I think simplifying the code to the giant match statement is best. Although 8 more nanoseconds per header check may seem a lot; in msecs it is 8e-6 (0.0000008 msecs). But I'll leave the decision in your capable hands.

I tested it and it works in my wasm app in debug mode :)

@seanmonstar seanmonstar changed the title Refactored header name matching Refactored header name matching for WASM target Jul 26, 2019
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for testing out all the tiny tweaks!

@seanmonstar seanmonstar merged commit d28a629 into hyperium:master Jul 26, 2019
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.

None yet

3 participants