add cache

[kittowned]

Added cache mechanism to address the issues https://github.com/yoavbls/pretty-ts-errors/issues/58 and https://github.com/yoavbls/pretty-ts-errors/issues/36

After adding the cache and benchmarking with performance.now() I was able to get roughly 10x performance improvement on large TS files. It's not a perfect solution however, since the cache uses a combination of the diagnostic range and ts error code to differentiate between diagnostics, when modifying a TS file by pushing code down a line all diagnostics below that line change their ranges and as a result they are not found in the cache. Any ideas to improve this are very welcome.

I've also taken several profiles of the extension and most of the time is spent in markdown generation. Specifically in dedent and prettify libraries. When replacing those libraries with simple functions that do nothing and just return the strings extension performance improves significantly.

I'm not sure how to proceed from here, any advice will be greatly appreciated 😃 .

[yoavbls]

Hey @kittowned, Thank you for creating this PR. Solving the performance issues is super important, and you profiled it which is excellent. Thank you very much for that! I want to suggest another method for the caching instead of the Cache class. I thought about the caching keys a lot. As you find out, using the code + line ranges could be problematic in some cases because the lines are changing a lot. most of the errors would be with the same 4-5 error codes.

I think the best option would be a simple JavaScript Map with the whole error message string as keys. The keys would be very long, but it should be performant to find them and it doesn't require any calculations to generate them or find the values.

e.g:

const cache = new Map();
let formattedMessage = cache.get(diagnostic.message)

if(!formattedMessage) {
  // All the expensive formatting is happening here in formatDiagnosticMessage
  formattedMessage = formatDiagnosticMessage(diagnostic.message);
  cache.set(diagnostic.message, formattedMessage)
}

and if we want to limit the memory footprint and set the max size for the Map
we can remove stale values by adding something like this after the cache.set

if(cache.size > 20) { // or any size we want
  const firstCacheKey = cache.keys().next().value
  cache.delete(firstCacheKey)
}

Let me know what you think about it 🙂

[kittowned]

Hi @yoavbls ! Thank you very much for going over my PR, I really appreciate it!

I really like your elegant solution, I've went ahead and implemented it on my machine. In terms of performance this is definitely the way to go, we get all the previous gains from the cache and changing code lines doesn't invalidate the cache.

However, I did find a case that could be problematic, when there are more than one unique diagnostic messages, they show up on the first element that matches the message, please see screenshot with my contrived example:

I'm not sure this is a deal breaker because I don't see many users encountering this, but on the other hand it will crop up in places and could cause a bit of confusion. Something to consider. Please let me know your thoughts :)

[yoavbls]

@kittowned Thank you! I'm not sure precisely what is causing this behavior. I think we can use the cached formatted message separately for every error and avoid these duplications.| Can you commit your changes, please? I'll take a look 🙂

[kittowned]

Hi @yoavbls . Sure thing, committed 😃

[kittowned]

Hi @yoavbls ! Thank you for the detailed explanation. You are definitely right, I didn't consider separating the formatted message from the range in this way 😅 . I've pushed a new commit with these changes. Do you think we need to add a cache limit at this time?

[kittowned]

Hey @yoavbls , I've added a limit of 100 as you advised. Please merge at your leisure 😄

[yoavbls]

Thank you!! 🙂

[WesleyYue]

@yoavbls Would love to have this fix. Could you cut a new release with this fix? If not I can build myself as well.

[yoavbls]

@WesleyYue I plan to release a new one at the end of the following week but I can make a one-off version with this fix, do you want that?

[WesleyYue]

@yoavbls yeah that would be awesome. I've been turning this extension on only when I need it due to the performance issues. Really appreciate your work on this!

[yoavbls]

@WesleyYue v0.5 is out 🙂 https://github.com/yoavbls/pretty-ts-errors/releases/tag/v0.5.0