microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.61k stars 29.42k forks source link

fix: disable persistent and in-memory cache when loading resources in oss #234357

Closed deepak1556 closed 19 hours ago

deepak1556 commented 23 hours ago

Refs https://github.com/microsoft/vscode/issues/148541

bpasero commented 22 hours ago

I am actually wondering if this makes the development experience worse for engineers on slower machines. So curious if at all its possible to limit this to CSS files only? Why do we need to do changes in 2 files, wouldn't the one in protocol service suffice, where we could do it for CSS Only?

deepak1556 commented 22 hours ago

There are two types of caches one for persistent and other for in-memory. All resources will first be put into the in-memory cache and after a certain threshold they will get persisted to the disk cache to be read from. There is no finer control for which resource to not use the persistent cache with the current API exposed from the runtime, for the in the in-memory cache since I control it based on header I can isolate it to the resource types we want.

There wouldn't be a performance cost with disabling the persistent cache for oss since we load each resource from disk even without the cache. However, there will be cost when not using the in-memory cache. The issue refers to css files but this caching strategy applies to all resources.

Maybe as a first pass I target the fix per below,

1) Avoid in-memory cache for css files 2) Disable persistent cache globally

Tyriar commented 22 hours ago

@bpasero I've been sending trace logs and investigating with @deepak1556 the problem I've been seeing and this does seem to fix my JS files consistently not loading problem, so it should significantly improve my situation by allowing me to reload again and not accidentally test without changes which gets very frustrating.

deepak1556 commented 22 hours ago

Given @Tyriar experience, what if we provide a argv.json setting to control disabling the in-memory cache for provided resource types, for ex: disable-http-memory-cache: "js,css,img,html" ? Or we can just keep it simple disable-http-memory-cache: boolean

bpasero commented 22 hours ago

Curious: what if we were to file watch changes in OSS and then update the headers accordingly. Is it at all possible to fix this issue like a web server would, by invalidating a cache for a resource?

I am bringing this up because maybe we can fix this nicely enough on a per-file basis if possible?

deepak1556 commented 20 hours ago

Yeah what you are thinking should be done by the last-modified header, the backend in the runtime which performs file loading already sets this https://source.chromium.org/chromium/chromium/src/+/main:content/browser/loader/file_url_loader_factory.cc;l=728-731 based on the file modified time.

Based on the runtime trace,

Title | ResourceFetcher::DetermineRevalidationPolicy
Category | blink

policy | "use"
reason | "Use the existing resource because there is no reason not to do so."

we are not getting RevalidationPolicy::kRevalidate. I need to check why that is the case to come up with a cleaner solution https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc;l=2001-2190.

deepak1556 commented 20 hours ago

This smells to be the issue https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource.cc;l=437-440 which avoids calculating cache freshness based on the last-modified header https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc;l=2148-2186

deepak1556 commented 19 hours ago

Yeah that was it, I am able to see proper cache validation now. I will follow-up with a fix in the runtime.