Send parameterizable rewriter #224
Conversation
orium
force-pushed
the
send-parameterizable-rewriter
branch
2 times, most recently
from
eaa561a
to
abace12
Compare
| // implementations of `HandlerTypes`, so each implementation of `HandlerTypes` needs to prove | ||
| // that a handler compatible with itself is creatable. | ||
|
|
||
| fn new_end_tag_handler<'h>( |
There was a problem hiding this comment.
I'm still working at improving these methods.
edit: done (not much can be improved I think)
orium
force-pushed
the
send-parameterizable-rewriter
branch
5 times, most recently
from
df1d4ac
to
5feceab
Compare
orium
requested review from
jongiddy,
Noah-Kennedy,
scotchmist,
kflansburg,
jasnell and
a team
as code owners
orium
changed the title
src/rewriter/mod.rs
Outdated
| @@ -371,9 +380,41 @@ mod tests { | |||
| out | |||
| } | |||
|
|
|||
| #[allow(clippy::drop_non_drop)] | |||
| #[test] | |||
| fn handlers_covariance() { | |||
There was a problem hiding this comment.
What's the purpose of this test? It doesn't seem to have any assertions
There was a problem hiding this comment.
It checks the the handlers continue to be covariant, that is, if you have a handler with a lifetime larger than 'a you can put that handler in a place where a lifetime of 'a is expected.
The test doesn't assert anything because it is a "if it compile it is good". If the handlers stopped being covariant in their lifetime it test would not compile.
I'll add some comments.
| @@ -13,7 +13,7 @@ define_group!( | |||
|
|
|||
| Ok(()) | |||
| })], | |||
| ..Settings::default() | |||
| ..Settings::new() | |||
There was a problem hiding this comment.
Why changing this to new?
There was a problem hiding this comment.
Settings::new() creates settings for a non-Send rewriter (that is, it creates a Settings<'_, '_, HandlerNormalTypes>).
Settings::new_send() creates settings for a Send rewriter (that is, it creates a Settings<'_, '_, HandlerSendTypes>).
Settings::default() also creates settings for a non-Send rewriter, just like Settings::new().
The reason I added Settings::new() is because I had to add Settings::new_send() so it would make sense to also have Settings::new() as well.
src/lib.rs
Outdated
| @@ -37,12 +37,49 @@ use cfg_if::cfg_if; | |||
|
|
|||
| pub use self::rewriter::{ | |||
| rewrite_str, AsciiCompatibleEncoding, CommentHandler, DoctypeHandler, DocumentContentHandlers, | |||
| ElementContentHandlers, ElementHandler, EndHandler, EndTagHandler, HandlerResult, HtmlRewriter, | |||
| MemorySettings, RewriteStrSettings, Settings, TextHandler, | |||
| ElementContentHandlers, ElementHandler, EndHandler, EndTagHandler, HandlerNormalTypes, | |||
There was a problem hiding this comment.
Let's rename HandlerSendTypes and HanderNormalTypes to SendHandlerTypes and LocalHandlerTypes
| ) -> Self::ElementHandler<'h>; | ||
|
|
||
| /// Creates a handler by running multiple handlers in sequence. | ||
| fn combine_handlers(handlers: Vec<Self::EndTagHandler<'_>>) -> Self::EndTagHandler<'_>; |
There was a problem hiding this comment.
This seems to be used only internally, should we use #[doc(hidden)]?
src/selectors_vm/mod.rs
Outdated
| impl<E: ElementData> SelectorMatchingVm<E> { | ||
| impl<E: ElementData> SelectorMatchingVm<E> | ||
| where | ||
| E: Send, |
There was a problem hiding this comment.
Nit: move ElementData to the where clause
orium
force-pushed
the
send-parameterizable-rewriter
branch
from
5feceab
to
72656cc
Compare
Some data structures had to be turned into
Syncto make the rewriterSend(if the handlers areSend). This has a performance penalty even when using a non-Sendrewriter.The benchmarks have a median slowdown of -0.8%. You read that right: a negative slowdown, which is a speedup. This can only be explained by noise while running the benchmarks, as they can only get slightly slower, not faster. However, it shows that the performance change is negligible (in my x86 machine, at least).