feat: emit files on demand and cache emit in sqlite db #15198

dsherret · 2022-07-13T21:24:56Z

This change causes source files to be emitted on demand (emits should no longer get out of sync with sources except potentially with deno coverage), stores the emit cache in an sqlite database, and no longer stores the source maps inline in the emitted file (it only embeds source maps on the fly in a file if someone uses --inspect or --inspect-brk).

~~Test for TS file changing between running workers (should now work)~~ Edit: nope it doesn't because it gets the source from the cached module graph, which seems reasonable. Will have to look into this later.
Unit tests for emit sqlite cache
Update https://deno.land/x/deno_cache -- Will do this after getting the thumbs up on this.
Test for emit cache not being able to be created.

Closes #13302
Closes #15124

Breaking Change

The emit file is no longer included with deno info --json.

…o verify they're getting the right source

nayeemrmn · 2022-07-13T21:36:54Z

We should have an alternative way of inspecting emit

dsherret · 2022-07-13T21:39:52Z

@nayeemrmn we're going to add it to here: https://deno.land/x/deno_cache

Also, I'm thinking to develop a web server that can be run locally to look at the cache contents (maybe something in the deno_cache repo that can be run from the command line), but even just using something like DB Browser for SQLite is not too bad.

nayeemrmn · 2022-07-13T22:30:27Z

I inspect emit often, though usually for checking transpilation/cache bugs. My workflow of deno info temp.ts -> ctrl+click emit path can be replaced by dcache show-emit temp.ts, so I hope https://deno.land/x/deno_cache will at least have an installable CLI which can do that.

dsherret · 2022-07-13T22:52:45Z

@nayeemrmn yeah for sure. That would be really easy to do

dsherret · 2022-07-14T20:16:32Z

cli/module_loader.rs

 pub struct CliModuleLoader {
  pub lib: TsTypeLib,
  /// The initial set of permissions used to resolve the static imports in the
  /// worker. They are decoupled from the worker (dynamic) permissions since
  /// read access errors must be raised based on the parent thread permissions.
  pub root_permissions: Permissions,
  pub ps: ProcState,
+  // the emit cache can only be used on one thread due to sqlite constraints
+  emit_cache: EmitCache,


I had to move a lot out of proc_state and into this file because EmitCache can only be used on a single thread whereas ProcState gets sent around all over the place.

dsherret · 2022-07-14T20:31:47Z

cli/cache/emit.rs

+                ),
+                err
+              );
+              Self(None)


A nice thing is the code can work fine without an emit cache now.

Are there any tests that demonstrate this? Like a read only cache dir? Feels like creating a tmp dir without read permissions would be a good test to ensure that code can still be emitted, just not cached.

No, but that would be good. I will add one.

kitsonk

some thoughts and questions

cli/cache/disk_cache.rs

kitsonk · 2022-07-16T09:25:57Z

cli/cache/emit.rs

+                ),
+                err
+              );
+              Self(None)


Are there any tests that demonstrate this? Like a read only cache dir? Feels like creating a tmp dir without read permissions would be a good test to ensure that code can still be emitted, just not cached.

cli/cache/incremental.rs

kitsonk · 2022-07-16T09:30:45Z

cli/cache/mod.rs

      Some(CacheInfo {
        local: Some(local),
-        emit,
-        map,
+        // we no longer store the emit and map in their own files


this impacts the output of deno info mod.ts --json, which is a breaking change. It feels like this is something we should address in deno_graph or at least make a wider note of instead of sort of just stubbing it off.

Yes, see the bottom of the PR description. It was agreed upon that breaking this was worth it in order to fix the issues.

That said, an alternative to not be breaking is to store a hash of the emit in addition to the source in the current version file. Then we would load the source, versions (two hashes), and emit then check that the emit hash matches the one in versions and the source hash matches the one in versions. If they do, then we can use the emit. By breaking here the main gain we get is that we don't need to bother hashing the emit... the more I think about it the more I would prefer this instead actually and it would be less work, though still breaking for deno_cache (because of change to versions file). Thoughts?

Edit: Oh, there is also the source map hash. That could just be moved back into the file.

It would be really easy to pivot this PR to that actually. I think I will try it out on Monday and see how it performs. I'm getting a little concerned depending on sqlite so much.

I got a bit carried away and opened this PR that does this: #15220 -- I think it's better

cli/emit.rs

kitsonk · 2022-07-16T22:10:12Z

cli/tests/integration/info_tests.rs

-use test_util::TempDir;
-
-#[test]
-fn info_with_compiled_source() {


Why just remove the test... This is an strong indication that we are introducing a breaking change with deno info, something we should address.

Yes, it was removed because of the breaking change (address in PR description). This is testing if "emit: " is in the output on line 35 and that's no longer the case.

cli/text_encoding.rs

dsherret · 2022-07-18T14:52:46Z

Closing this in favour of #15220 as it's less breaking. Measured the performance of everything and both this and that PR have the same performance (about 15% faster than main on the test I was doing)

dsherret added 7 commits July 13, 2022 15:39

Start of work.

f4ebdfb

Merge branch 'main' into lazy_emit

1ac4ca0

More changes.

6e9707b

Fixes.

6a0c191

Clippy.

250722f

Verify the source hash in the cache in order to encourage consumers t…

85fc0be

…o verify they're getting the right source

Merge branch 'main' into lazy_emit

76adfda

dsherret added 5 commits July 14, 2022 11:16

Fix coverage code.

a4e0615

Emit cache unit tests

e83113b

Merge branch 'main' into lazy_emit

3ae3431

Reduce memory usage by storing source map separately.

ceede09

Lint.

adecc49

dsherret commented Jul 14, 2022

View reviewed changes

dsherret added 2 commits July 14, 2022 16:23

Updates.

87fb86e

Merge branch 'main' into lazy_emit

098f5b8

dsherret commented Jul 14, 2022

View reviewed changes

dsherret requested review from kitsonk and bartlomieju July 14, 2022 20:35

dsherret marked this pull request as ready for review July 14, 2022 20:35

dsherret added 3 commits July 14, 2022 16:48

Remove logic for optional source hash as it's now unnecessary.

43c090b

Merge branch 'main' into lazy_emit

a137bea

Merge branch 'main' into lazy_emit

2a131c7

kitsonk reviewed Jul 16, 2022

View reviewed changes

dsherret added 3 commits July 16, 2022 18:39

Merge branch 'main' into lazy_emit

b01b98f

This should actually be the opposite way

0d53b1d

Hash -> Hasher

4fb035e

dsherret mentioned this pull request Jul 17, 2022

feat: emit files on demand and fix racy emit #15220

Merged

1 task

dsherret closed this Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: emit files on demand and cache emit in sqlite db #15198

feat: emit files on demand and cache emit in sqlite db #15198

dsherret commented Jul 13, 2022 •

edited

Loading

nayeemrmn commented Jul 13, 2022

dsherret commented Jul 13, 2022 •

edited

Loading

nayeemrmn commented Jul 13, 2022 •

edited

Loading

dsherret commented Jul 13, 2022

dsherret Jul 14, 2022

dsherret Jul 14, 2022

kitsonk Jul 16, 2022

dsherret Jul 16, 2022

kitsonk left a comment

kitsonk Jul 16, 2022

kitsonk Jul 16, 2022

dsherret Jul 16, 2022 •

edited

Loading

dsherret Jul 16, 2022

dsherret Jul 17, 2022

kitsonk Jul 16, 2022

dsherret Jul 16, 2022

dsherret commented Jul 18, 2022 •

edited

Loading

feat: emit files on demand and cache emit in sqlite db #15198

feat: emit files on demand and cache emit in sqlite db #15198

Conversation

dsherret commented Jul 13, 2022 • edited Loading

Breaking Change

nayeemrmn commented Jul 13, 2022

dsherret commented Jul 13, 2022 • edited Loading

nayeemrmn commented Jul 13, 2022 • edited Loading

dsherret commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret Jul 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret commented Jul 18, 2022 • edited Loading

dsherret commented Jul 13, 2022 •

edited

Loading

dsherret commented Jul 13, 2022 •

edited

Loading

nayeemrmn commented Jul 13, 2022 •

edited

Loading

dsherret Jul 16, 2022 •

edited

Loading

dsherret commented Jul 18, 2022 •

edited

Loading