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

Add specialization #167

Closed
wants to merge 0 commits into from
Closed

Add specialization #167

wants to merge 0 commits into from

Conversation

zzau13
Copy link
Contributor

@zzau13 zzau13 commented Dec 4, 2018

Nightly

Big table               time:   [280.96 us 281.06 us 281.20 us]
                        change: [-56.768% -56.745% -56.721%] (p = 0.00 < 0.05)

Teams                   time:   [485.02 ns 486.16 ns 487.31 ns]
                        change: [-43.564% -43.426% -43.294%] (p = 0.00 < 0.05)

@zzau13
Copy link
Contributor Author

zzau13 commented Dec 4, 2018

At stable, the average escape time has been improved, remove the false positives, ascii 34 to 62, by means of a static table, it does not use simd.

At nightly, simd has been integrated along with specialization, reduction of false positives to 1/64, full coverage and optimizations.

@djc
Copy link
Owner

djc commented Dec 7, 2018

So that's a big improvement on nightly, but it looks like the improvement comes largely from specialization (more so than from the SIMD implementations). I'm still unconvinced about the benefits of SIMD by itself, and also not sure about the complexity here.

Here's some alternative thinking: what if we can make turn MarkupDisplay into a trait such that escaping is more pluggable? We should solve #108 and #136 at the same time as allowing you to plug in your custom escaping code.

@zzau13
Copy link
Contributor Author

zzau13 commented Dec 7, 2018

I agree, the best improvement in this e2e is offered by the specialization.

The complexity is high but the performance is 10% below memchr3. It also has a relative coverage of 100% and only works at nightly. In stable it works by static table with what gives better performance for 2 or more false positives than the previous one.

What do you propose for SIMD? for the results of other implementations, it will not have much better performance. We leave it or what else can I do?

As for the trait, we can be done but we can only implement it on primitive types (usize, i8, str, String, ..), so it will stop working in other structures that imp Display. That or I do not just understand you.

As for the #108 and #136, I do not understand it very well, I had thought about the specialization for these. You can explain a little more about this solution for the this issues.

@zzau13
Copy link
Contributor Author

zzau13 commented Dec 8, 2018

I enclose the benchmarks with throughput , they are the same data that rust-memchr

@zzau13 zzau13 changed the title Add v_htmlescape with specialization [WIP] Add v_htmlescape with specialization Dec 10, 2018
@zzau13
Copy link
Contributor Author

zzau13 commented Dec 10, 2018

The main implementation of SIMD, avx, loses performance below 11 bytes.

I propose an operator for expressions, such as the WS, to use one or the other depending on whether the operator is or not. Also integrate it so that you can choose different types of escape, latex ...., which could also use SIMD with this abstraction of the main loop.

As for the issue #136 , I can write a static table at compile time from askama.toml with the scalar implementation. With simd it is more complicated, but it can also be done simply by calculating the 3 minimum ranges, later like the previous one.

SIMD uses only at nightly.

@djc
Copy link
Owner

djc commented Dec 11, 2018

I'm sorry -- I don't have the energy right now to dig into this, and I sometimes find it hard to understand what exactly you mean; there's a bit of a language barrier still.

By the implementation of #136/#108 I mean that I think it would be useful to parametrize Template and MarkupDisplay with a type argument that's bound by an Escaper trait which has a single escape() method (similar to the current MarkupDisplay::escape() method). Instead of just a table of characters to escape in askama.toml, we would then configure the name of the Escaper impl to use, like this:

[escape]
html = "v_escape::HtmlEscaper"

If nothing is configured, it would default to the askama_escape::HtmlEscaper.

@zzau13 zzau13 changed the title [WIP] Add v_htmlescape with specialization [WIP] Add specialization Dec 12, 2018
@zzau13
Copy link
Contributor Author

zzau13 commented Dec 12, 2018

Ok, I only added the specialization.

If #136/#108, I have it ready for the next one.

@zzau13 zzau13 changed the title [WIP] Add specialization Add specialization Dec 12, 2018
@zzau13
Copy link
Contributor Author

zzau13 commented Dec 13, 2018

I know, damn English, I need linguistic immersion. What I'm trying to say: let's merge the specialization and then we fix this issues.

@zzau13 zzau13 closed this Dec 20, 2018
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