Faster Scanner (part 2) #54
Conversation
Original time: 1.09s |
|
Nice work on this. Having removed the use of closures, would it be straightforward to get this compiling without C++14 features? That would make this parser a bit easier to consume in downstream applications, and develop on a Mac. |
|
awesome!! |
Yes I plan to remove the need for c++14, and maybe even convert the scanner to pure C :) |
|
Oh, that’s interesting. I’d think that C++ would be useful just for std::vector. are you thinking that a dynamically sized array isn’t needed in the scanner? |
I think we will need one, but I'm happy to implement my own if it means we can compile this as C. While we have you here, would we be able to get your opinion on #41 (comment)? We think the scanner's probably running over the whole file (or a lot of it) every edit, but we're not sure if it should. |
|
@wenkokke I've removed all the closures. It now compiles with I'll see what I can do. |
Original time: 1.09s There might be one or two micro-optimizations left (PEEKing before taking some code paths?) But this will in any case let us edit 36x larger files than before 🎉 |
|
With your current faster-scanner branch, you've got the patch to tree-sitter down to: --- a/lib/binding_web/exports.json
+++ b/lib/binding_web/exports.json
@@ -3,6 +3,9 @@
"_free",
"_malloc",
+ "__ZNSt3__25ctypeIcE2idE",
+ "__ZNSt3__212basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE25__init_copy_ctor_externalEPKcm",
+
"__ZNKSt3__212basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE4copyEPcmm",
"__ZNKSt3__220__vector_base_commonILb1EE20__throw_length_errorEv",
"__ZNSt3__212basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE6__initEPKcm",See https://github.com/wenkokke/tree-sitter-haskell/tree/tree-sitter-haskell-wasm-faster-scanner. |
|
@tek This is ready for review, when you have a chance. All tests pass. Any further changes will probably be cosmetic (moving things around, rather than changing what they do). |
|
awesome! |
| peek('i')(in) | ||
| )(state); | ||
| // TODO Convert to switch | ||
| if (cond::symbolic(PEEK) || PEEK == '`') { |
There was a problem hiding this comment.
2 peeks happen here, store in a variable? Or use a switch(PEEK) like the TODO mentions :)
| string mark_target = "consume_until " + target; | ||
| util::mark(mark_target, state); | ||
| while (PEEK != 0 && !seq_v2(target, state)) { | ||
| while (PEEK != 0 && PEEK != first) S_ADVANCE; |
There was a problem hiding this comment.
2 PEEKs, store in a var to only PEEK once?
|
@luc-tielen I appreciate the comments, but I think a lot of them are out of scope, either because I haven't changed the code from what it was doing before, or because they're microoptimizations that the compiler may or may not already be doing. I'd be interested to see if there's a performance win by implementing your |
|
@414owen That's ok. Your PR was fine, so I could only point out the nitpicky stuff. |
|
@414owen it appears to get stuck in an infinite loop when parsing HLS for me, can you confirm? |
I'm building helix atm. I'll run it in gdb and see where :) |
|
Okay I think I need to check for an |
|
|
crazy |
See part 1 for more info.
Overall speedup: around 45x.