Switch from kuchiki to LOL_HTML

[jyn514]

Closes #876

My styles haven't been working since ~forever, if someone else could check this looks right that would be great. However this looks approximately the same before and after to me.

Ideas for testing this are also welcome - the previous tests were only testing the parsing, not the rendered code, so they're not very useful here.

Before:

After:

r? @Kixiron
cc @nataliescottdavidson

[jyn514]

Source before: https://gist.github.com/jyn514/421101e65a28bb67720f0fd31628b4c6
Source after: https://gist.github.com/jyn514/08ee9831ad5cb853cb528adb6aa2ea37

[jyn514]

Going to try this on the 300 MB file and see what happens.

[jyn514]

Once cloudflare/lol-html#56 is fixed, we could also remove SizedBuffer and use MemorySettings instead (technically we could do it earlier but I'm lazy). That can wait for a follow-up PR though.

[jyn514]

Going to try this on the 300 MB file and see what happens.

It worked 🎉 🎉 🎉 Took about a minute to serve then another 5 to load but that's to be expected :)

[jyn514]

It used ~350 MB of memory to serve which seems about right.

[Kixiron]

This looks like a really great change, makes me wonder if we shouldn't add a time limit of some sort later on down the line to prevent us getting swamped (alternatively having a higher size cap would probably work, letting us not even try to parse files that are over x MB). You may need to update the benchmarks, and I'd be really interested to know if there's a speedup of any sort after the switch

[jyn514]

makes me wonder if we shouldn't add a time limit of some sort later on down the line to prevent us getting swamped (alternatively having a higher size cap would probably work, letting us not even try to parse files that are over x MB)

For now I left the cap at 5 MB. I planned to make a follow-up PR removing SizedBuffer in favor of memory limits, but since you mentioned it I'll make it part of this one.

You may need to update the benchmarks, and I'd be really interested to know if there's a speedup of any sort after the switch

Good point, I forgot to update them. The benchmarks are not going to be one-to-one though - before we only measured the parse time, while now it measures both parsing and templating (and that's sort of a fundamental part of this change, there's not a way to do one without the other).

[Kixiron]

LGTM modulo nits, great job!

[jyn514]

@Kixiron how set are you on having benchmarks? So far I have

    let html = std::fs::read_to_string("benches/struct.CaptureMatches.html").unwrap();
    let page = RustdocPage {

    };
    let config = unimplemented!();
    let conn = Connection::connect(config.database_url.as_str(), postgres::TlsMode::None)?;
    let ctx = Context::from_serialize(&page).unwrap();
    let templates = TemplateData::new(conn).unwrap();

and it shows no signs of slowing down, I just need more and more things to be public.

[Kixiron]

Benches don't matter a ton, I can follow-up with them if need be

[jyn514]

I also raised the default HTML size limit from 5 MB to 50 MB. Since I tested on a 300 MB file this shouldn't give the prod server any problems.

[inikulin]

@jyn514 If understand the context correctly then 5Mb should be enough. Lol-html requires to allocate memory only for really long tags (without content, just start tag tokens). So if you don't have/don't plan to have start tags longer than 5Mb than it should be more than enough for the observable future. (just in case: increasing the buffer size doesn't boost performance)

[jyn514]

@jyn514 If understand the context correctly then 5Mb should be enough. Lol-html requires to allocate memory only for really long tags (without content, just start tag tokens). So if you don't have/don't plan to have start tags longer than 5Mb than it should be more than enough for the observable future. (just in case: increasing the buffer size doesn't boost performance)

Wait, did I read that right? Lol-html will only allocate as much memory as the size of the start tag, even for multi-megabyte files? That's amazing!

[inikulin]

@jyn514 yep, it's a streaming parser, so we allocate as less as possible

[jyn514]

Switched the limit to 5 MB and it still loaded the 300 MB file fine 🎉

[jyn514]

Aha, I was looking at the wrong screen! Those are some nice numbers :D

[inikulin]

@jyn514 btw, I believe you can push the memory limit even further to 3-5Kb, I believe in your case that should be enough