tree-sitter / tree-sitter-python

Python grammar for tree-sitter
MIT License
372 stars 138 forks source link

fix: do not peek beyond comments if indent, dedent, or newline aren't valid #242

Closed amaanq closed 1 year ago

amaanq commented 1 year ago

Closes https://github.com/nvim-treesitter/nvim-treesitter/issues/4839

The methodology used to debug this performance regression is similar to https://github.com/tree-sitter/tree-sitter/pull/2637 in using perf, this will be slightly less in-depth to avoid repeating myself.

This fixes a bug when reparsing ranges where a comment might be in the event of checking for an indent, dedent, or newline.

To observe the bug, compile tree-sitter-python manually w/ debug symbols and place that in your parser rtp for neovim, open the reproducer file which is provided in the issue listed above, and hit o on line 1280 or 1281, you should notice a delay when reparsing occurs. If you hit o after the import on line 1282, you will not notice this delay.

When using perf, I attach to neovim by first getting the PID and manually attaching to it:

image

Once done, you can use a GUI tool like hotspot (I really recommend using it, it's an awesome tool) instead of the cli to observe where issues arise, I'll use the cli to show points of interest:

image

Now using hotspot, we can take a look more easily without mastering the cli app, which gives us a "Top Hotspots" list, similar to the cli:

image

Let's right click on ts_lexer__do_advance and hit view caller/callee.

We can see in the caller table it calls skip, which seems to be one of the sources of slowness, which feels weird. If we navigate to skip, its caller is "tree_sitter_python_external_scanner_scan", so it seems the scanner seems to be causing the high cpu cycle count in skip. Let's dig into its assembly by clicking on it:

image

On the right hand side of the source code and disassembly windows, youll see percentages marking the % of cpu cycles spent on a block/line of code. Here's where we see a high amount:

image

It is weird that skip seems to cost a lot of cpu cycles, but thinking it over, it seems we don't always want to advance past comments and peek to see the next indentation length. This only matters IF the indent, dedent, or newline symbols are valid, a string_start is not considered valid if the comment indentation length is not none (-1). So, we can simply check for the validity of those 3 symbols to avoid always endlessly skipping past comments.

The high cpu cycle count in skip is weird, but I couldn't reason about as to why that is - perhaps it's purely a side effect of always skipping past comments here :shrug:

What I did find most concerning is this single line in __do_advance costing 10% of the cpu cycles (because skip is true when advancing past a comment to peek at indentation):

image

That's at line 190 of lexer.c

It might be something to do with cache misses, or some other weird assembly stuff.. @ahlinc @maxbrunsfeld this might interest you guys

But, with the simple check of valid_symbols, the performance issue is most definitely gone :grin: