perf: v8 code cache #23081
perf: v8 code cache #23081
Conversation
| @@ -14,4 +14,4 @@ setTimeout(async () => { | |||
|
|
|||
| setTimeout(() => { | |||
| console.log("Success"); | |||
| }, 50); | |||
| }, 200); | |||
There was a problem hiding this comment.
@bartlomieju - I needed to change this to make this test pass on windows. otherwise Success is printed too early. perhaps code cache changed the execution timing a bit and 50ms wasn't enough.
There was a problem hiding this comment.
Is it possible it was a flake on Windows? I don't see anything in your PR that should affect this. This smells like a deno_core bug.
I'm also not sure why we are using an async function in the setTimeout above. That could be part of the problem.
There was a problem hiding this comment.
It's possible that it flaked on Windows; it failed consistently for several runs though. I'll try to do more runs with/without this change.
There was a problem hiding this comment.
I confirmed again that the test is consistently failing on windows, and a higher timeout makes the output appear as expected.
cli/module_loader.rs
Outdated
| .get_module_source_hash(specifier, code_source.media_type) | ||
| .ok() | ||
| .flatten(); | ||
| let code_timestamp = specifier_to_file_path(specifier) |
There was a problem hiding this comment.
Do we actually need the timestamp for code cache? Should the source hash be sufficient?
What would happen if we just avoided code cache for modules that didn't have a source cache in the ModuleInfoCache?
There was a problem hiding this comment.
Do we actually need the timestamp for code cache? Should the source hash be sufficient?
2 reasons:
- For some modules
ModuleInfoCachedoesn't provide the source hash - The other part of this PR deals with code cache for
op_eval_contextscripts, for which we don't compute/maintain hashes. Sinceop_eval_contextscripts always come from the local file system, it seems that we could rely on timestamps rather than adding additional overhead to compute hashes.
What would happen if we just avoided code cache for modules that didn't have a source cache in the ModuleInfoCache?
Yes, I think we could do that for ES modules. And then we can make it such that ES modules provide a hash, while op_eval_context scripts provide a timestamp.
There was a problem hiding this comment.
Imported npm modules don't have source hash in ModuleInfoCache. I'm assuming that's intentional and we don't want to introduce hash computation for them. I'm making a change to fallback to timestamps in that case.
There was a problem hiding this comment.
The other part of this PR deals with code cache for op_eval_context scripts, for which we don't compute/maintain hashes. Since op_eval_context scripts always come from the local file system, it seems that we could rely on timestamps rather than adding additional overhead to compute hashes.
That's not entirely true, the other case is that when you require() something we use op_eval_context that has a generated source code (that wraps something loaded from the FS).
There was a problem hiding this comment.
For this case, what is the specifier?
There was a problem hiding this comment.
Ah in this particular case it will be file:///<original specifier> so I guess you're right here.
| @@ -517,6 +517,7 @@ pub struct Flags { | |||
| pub unstable_config: UnstableConfig, | |||
| pub unsafely_ignore_certificate_errors: Option<Vec<String>>, | |||
| pub v8_flags: Vec<String>, | |||
| pub code_cache_enabled: bool, | |||
There was a problem hiding this comment.
We do have negative-sense flags in this flags struct -- see no_npm, no_prompt, etc. It would simplify the test changes required for this file.
There was a problem hiding this comment.
Unfortunately big test change will still be needed in this file because we're only enabling code cache for run command initially.
There was a problem hiding this comment.
I ended up keeping this the same. With the negative no_code_cache flag there are actually more test changes needed (because code cache is disabled for non-run commands).
|
@igorzi looks like you accidently committed WPT update. Please revert, other than that LGTM |
This PR enables V8 code cache for ES modules and for `require` scripts through `op_eval_context`. Code cache artifacts are transparently stored and fetched using sqlite db and are passed to V8. `--no-code-cache` can be used to disable. --------- Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
This PR enables V8 code cache for ES modules and for
requirescripts throughop_eval_context. Code cache artifacts are transparently stored and fetched using sqlite db and are passed to V8.--no-code-cachecan be used to disable.