fix(compiler): Correct parsetree caching behaviour#2280
Conversation
| Option.fold(~none=None, ~some=Hashtbl.find_opt(cached_parsetrees), name) | ||
| ) { | ||
| | Some((cached_source, cached_program)) | ||
| when cached_source == Hashtbl.hash(source) => |
There was a problem hiding this comment.
Hashing the entire parsetree is super expensive. I would use file_older or some other mechanism to verify that the file hasn't changed and that the cache is valid.
There was a problem hiding this comment.
Hashing the entire parsetree is super expensive. I would use
file_olderor some other mechanism to verify that the file hasn't changed and that the cache is valid.
I'm only hashing the source which is just hashing a string right? I am not against using file_older though but I don't think that would work here because of .compile_string caching
There was a problem hiding this comment.
That's not as bad; I thought it was hashing the AST.
There was a problem hiding this comment.
Nope I am essentially just using the source hash to validate if the contents of the name has changed and we should be using a new parsetree rather than the cache.
There was a problem hiding this comment.
I think it would be useful to test this with a large file (maybe like 25k-50k lines) and make sure the hashing isn't really noticeable compared to the parsing. Also, we should make an issue to only keep a number of cached parsetrees, because this is basically a memory leak in its current form.
There was a problem hiding this comment.
We do not. They're both O(n), but remember that runtime complexity just describes how performance of the algorithm grows with more input, not actual running. You can have two O(n) algorithms, but one might take 20x longer than the other one. Both being O(n) just means that with more data, that one will still only take 20x longer than the other one.
Strings are easy to hash and have quick, optimized algorithms, whereas hashing a big data structure requires chasing a bunch of pointers, offset calculations, etc. ASTs are also a lot more data than the source strings.
There was a problem hiding this comment.
Coming back to this, do you think it would be acceptable to test with Unix.time() and just ensure the time taken in A is less than B?
My concern with this approach is if the times are too close, a cpu hickup could cause our tests to flake.
There was a problem hiding this comment.
Apologies if you thought I meant I wanted perf tests in the test suite. I just want benchmarks that you ran on your machine and you report back on the numbers.
There was a problem hiding this comment.
The results from testing this are below, it seems that it's way faster to have the cache, the top image is with the caching logic implemented and the bottom is without the caching logic enabled.
The reason I mentioned the O(n) above was just a note on scaling that if the cache is that much faster for smaller files or larger files it should scale relatively the same.
I agree that we should open an issue about leaking parsetrees I think when we delete them though, is a complicated question. We may also want to consider skipping the hash altogether for short inputs, we could do a very quick check of the byte length of the string, and if it's under a threshold, we just don't cache. I'll open that issue whenever this is ready to be merged.
As just one more note, I think we should avoid using file_older where possible to try and reduce the compilers dependence on a file system.
7cb911d to
6d70250
Compare
This corrects the behaviour of parsetree caching, we noticed the other day that the lsp wasn't updating prompting #2267. This corrects the root source of that bug by validating the source hashes match when we are caching.
I think with this change we would be safe to remove the compiler reset allowing the lsp to run a little faster however I don't want todo that in this pr as I think we still need to validate the behaviour of the other things we don't reset
DependencyGraphandEnv,Ctype levels,Warnings,FS_Access, testing locally everything was working again though.Note: After I made this pr I realized we use
file_olderin the dependencytree to check if something is dirty I'm wondering if instead of my source hashing semantics it would be better to use that api but I would like a bit of feedback before I go and do that.