chore(PLA-2392): do not attempt inlining for very deeploy nested html strings#11
chore(PLA-2392): do not attempt inlining for very deeploy nested html strings#11jareddellitt merged 7 commits intomainfrom
Conversation
| [package] | ||
| name = "css_inline_nif" | ||
| version = "0.1.2" | ||
| version = "0.2.0" |
There was a problem hiding this comment.
Bumping to next minor version because of the nature of this change
| @@ -17,4 +17,4 @@ nif_version_2_17 = ["rustler/nif_version_2_17"] | |||
|
|
|||
| [dependencies] | |||
| rustler = "0.37" | |||
| css-inline = "0.19.1" | |||
| css-inline = "0.20.0" | |||
There was a problem hiding this comment.
There's a new version of css-inline, so went ahead an pulled it in. Some minor performance improvements and a bug fix.
native/css_inline_nif/src/lib.rs
Outdated
| /// since BEAM-managed data must live on the BEAM heap. | ||
| #[rustler::nif(schedule = "DirtyCpu")] | ||
| fn inline_css(html: &str, opts: Options) -> Result<Vec<u8>, RustlerError> { | ||
| if exceeds_nesting_depth(html.as_bytes(), MAX_NESTING_DEPTH) { |
There was a problem hiding this comment.
Curious if we want to expose exceeds_nesting_depth/2 to Elixir and make calls to it outside of inline_css? That allows for feature flagging and perhaps dynamic control of MAX_NESTING_DEPTH?
There was a problem hiding this comment.
I thought about that, but without a use case opted not to. Maybe we could just make the max depth an option that's passed in and use it if it's given, else fallback to the statically defined max?
There was a problem hiding this comment.
How about this - make the max depth an option that's passed in:
unset == use the default-1 == skip the check- Otherwise use the value passed in
Just thinking about ways to incrementally roll this out on our critical path.
There was a problem hiding this comment.
Added an option to check depth and another for the actual depth.
| const MAX_NESTING_DEPTH: usize = 128; | ||
|
|
||
| /// Returns `true` if the DOM nesting depth exceeds `limit` via a fast byte | ||
| /// scan, without parsing. Counts `<x` as an open and `</` as a close; |
There was a problem hiding this comment.
Nit: thoughts on viewing /> as close too? That helps avoid the depth limit issues around tons of e.g. <br/> tags?
There was a problem hiding this comment.
The overcounting from void elements is already factored into the 128 threshold. The scanner's job is to be a cheap, conservative pre-pass and not an accurate depth counter. It would technically be more accurate, but we'd increase the complexity...
2 participants
Description
Adds a fast depth check before attempting inlining and returns an error if the maximum is reached. The function doing the checking isn't doing any allocations, and is an O(n) algorithm that bails early if the max has been hit so it doesn't have to scan the whole doc. For smaller docs it will read everything, but impact should be negligible because most aren't anywhere near as long as we saw with the pathological case.