Fix lexer being extremely slow on large amounts of whitespace. Fixes #857

[petee-d]

Fix for #857, where parsing whitespace in templates usually has quadratic time complexity.

It addresses two pathological regular expressions used in Jinja lexer, "root" and TOKEN_RAW_BEGIN (not illustrated in examples but suffers from the same issue). In the extremely simplified version of the root lexer regex shown below, notice whitespace may be consumed either by (.*?) or by \s*, with the latter having priority due to being greedy, but only in case the block of whitespace is followed by variable (or block/comment) opening tag with the "-" modifier (that forces left-stripping whitespace). This is a pathological case for the built-in Python re module and will have quadratic complexity due to backtracking done after consuming every single whitespace character (don't quote me on that though).

r'(.*?)(?:(?P<variable_begin>\s*{{-|{{))'

Jinja lexer does that to implement the whitespace stripping feature on the left side of tags, but this approach is extremely inefficient with built in Python re module (note the regex library doesn't suffer from this, but it is slower in general).

My approach uses .rstrip or .rfind + another regular expression to remove the whitespace inside the tokenizer.

Potential improvements

• I found the code to be quite hacky, especially the inspection of type of tokens in method tokeniter (shouldn't it be tokenizer btw?), when I needed to add a flag for a rule to attempt l-stripping, I saw no better way to continue in that pattern if I wanted to avoid larger refactoring. Any ideas?
• Is this project still sticking to 80 character maximum per line, as lexer.py apparently does? It makes the code very ugly and I see some modules no longer adhere to that limit. Is 120 character line maximum acceptable?
• It feels like the code somewhat avoids private helper methods. I especially feel like my new code block for l-stripping should be in such a separate method and I will be happy to move it to one unless you are actually trying to avoid the overhead of calling methods (which in my opinion should be negligible here).

Testing and performance measurement

I tested the fix on almost 100k of different Jinja2 templates, created by users with different technical skills and used for various purposes (from templates without any jinja tags to huge personalized email templates). Parsing all templates produced the exact same parsing tree both before and after my fix. The Jinja2 test suite of course passes as well.

The table below shows the parsing times of the templates I tested the fix with, each parsing attempt before or after was added to a bucket based on rounded log2 of the parsing time. You can see the 4 extremely slow cases being fixed and a general improvement of parsing time.

Seconds | Before | After
0.00006 |    873 |  1775
0.00012 |  19690 | 21348
0.00024 |   8805 |  9151
0.00049 |   5294 |  5331
0.00098 |   3603 |  2146
0.00195 |   2002 |  4824
0.00391 |   3109 |  8404
0.00781 |   7778 |  7759
0.01563 |   8455 |  3419
0.03125 |   4889 |  2234
0.06250 |   1711 |    63
0.12500 |    177 |     3
0.25000 |     67 |     0
0.50000 |      0 |     0
1.00000 |      0 |     0
2.00000 |      0 |     0
4.00000 |      2 |     0
8.00000 |      2 |     0

I didn't add any test case. I could add a test that parses something like (' ' * 40000) and asserts the time it took didn't cross a reasonably large boundary, but I generally dislike such tests as they may be fragile to CI environment issues. Let me know what you think.

[petee-d]

Note the build failed on something unrelated to my changes (Python 3.3.6 environment creation and it passes locally for me), could someone with repo access try to restart it or fix the underlying issue?

[davidism]

Sorry for letting this sit so long! I've marked this for 2.11. Unfortunately, there have been some other changes to the lexer and lstrip_blocks, and this doesn't merge cleanly. If you see this and can address this, let me know, as you understand the changes better than me.

[davidism]

I rebased, and it seems like your fix for this also addressed #748, or at least it passes the tests added by #1014. I'll check a little more.

[petee-d]

Oh, hey there. I'm afraid I only barely remember the changes I have done almost year and a half ago, but can help if needed. I understand you already rebased this (without pushing I guess), so I won't dive into it unless you say you'd like me to.

BTW, I would be quite happy if this got merged (or otherwise fixed), our project was using a fork with my fix ever since I made it and being able to update Jinja would be nice.

[davidism]

No problem, I think I have a handle on it, I'll try to push and merge later today.

[davidism]

For some reason I couldn't push my rebase to the source repo for this PR, but GitHub happily let me merge and commit to it using the online editor. 🤷‍♂️

I reorganized the code a bit and added a ton of comments in case I ever need to revisit this.