Codemirror benchmarks #150
Conversation
| @@ -220,7 +220,8 @@ class ParserWrapper { | |||
| parse(query: string): EnrichedParsingResult { | |||
| if ( | |||
| this.parsingResult !== undefined && | |||
| this.parsingResult.query === query | |||
| this.parsingResult.query === query && | |||
| process.env.BENCHMARKING !== 'true' | |||
There was a problem hiding this comment.
I'm not a fan of this cause if someone had this env variable defined (unlikely but still possible) then the behaviour of the language server changes.
I'd rather probably we had a method to clean the cache and we call that after every test of the benchmark?
There was a problem hiding this comment.
Wdyt about naming it something where it is more obvious the behavior of the language server changes, something like: BENCHMARK_CYPHER_LANGUAGE_SERVER?
I'm fine with having a method to clean the cache as well if you prefer
There was a problem hiding this comment.
Wdyt about naming it something where it is more obvious the behavior of the language server changes, something like: BENCHMARK_CYPHER_LANGUAGE_SERVER?
I think explicitly cleaning the cache is a better idea IMO. The reason is right now I'm not sure whether this is benchmarking the right thing because in the auto-completion or the syntax validation we are also including another component, which is the parsing. So I'd rather we could control when to clean or not to clean the cache
There was a problem hiding this comment.
I'm flexible, so if you prefer to do that on another PR, we can iterate on this. That way we get to test it out as well
There was a problem hiding this comment.
I think it'd be nice to do in a follow up PR, just to see that the Github actions work
There was a problem hiding this comment.
I went ahead and did it directly in this PR instead.
| @@ -300,3 +300,7 @@ export function applySyntaxColouring( | |||
|
|
|||
| return result; | |||
| } | |||
|
|
|||
| export function clearCache() { | |||
| delete parserWrapper.parsingResult; | |||
There was a problem hiding this comment.
Shouldn't this call parserWrapper.clearCache instead?
There was a problem hiding this comment.
It got in by mistake, thanks for catching it!
This PR includes two new github actions, one running on merged PR that records benchmarkresults to a grafana cloud instance and one job that compares main / pr benchmarks to warn if performance is decreased bellow a set threshold.
Note that the comparative github action will fail on this PR because there's no benchmarking script to compare with yet (as it's not merged yet). I tested the action comparing the PR to PR, which succeeded