Add --prefetch flag for deps prefetch without running #1475

kevinkassimo · 2019-01-07T05:22:35Z

Aiming to close #1386. This is built on top of #1460

The only difference from running is that the modules are compiled but NOT evaluated

kevinkassimo · 2019-01-07T05:24:43Z

libdeno/binding.cc

+  if (is_prefetch) {
+    return true;
+  }
+
  auto result = module->Evaluate(context);


The only modified part of this PR: module->Evaluate is not run and short circuited by is_prefetch

ry · 2019-01-07T16:22:50Z

Great - very glad you took this on! I will review after #1460 has landed

kevinkassimo · 2019-01-07T19:14:41Z

@ry Found a mysterious issue that I am not quite sure about during debugging:
Notice that the current diff test has surprising output (with --prefetch and --reload flags):

Compiling [WILDCARD]tests/prefetch.ts
Downloading http://localhost:4545/tests/subdir/subdir3/mod1.ts... << These 3 lines...
Downloading http://localhost:4545/tests/subdir/subdir3/mod2.ts... << These 3 lines...
Downloading http://localhost:4545/tests/subdir/subdir3/mod3.ts... << These 3 lines...
Downloading http://localhost:4545/tests/subdir/subdir3/mod1.ts...
Compiling http://localhost:4545/tests/subdir/subdir3/mod1.ts
Downloading http://localhost:4545/tests/subdir/subdir3/mod2.ts...
Compiling http://localhost:4545/tests/subdir/subdir3/mod2.ts
Downloading http://localhost:4545/tests/subdir/subdir3/mod3.ts...
Compiling http://localhost:4545/tests/subdir/subdir3/mod3.ts

The 3 lines marked are completely unexpected (since they download the files but not compiling them, and is duplicate with later downloads).
When I trace down where these downloads happens, it is exactly from here:

deno/js/compiler.ts

Line 405 in bc2c808

const output = service.getEmitOutput(fileName);

which surprised me since this is a TypeScript compiler builtin. Notice that resolveModule is the only part where we have the codeFetch logic injected and is used in resolveModuleNames

When I renamed resolveModuleNames to _resolveModuleNames, the unexpected downloads goes away...

I would assume that we unexpectedly overwrote a TypeScript API? It seems that getEmitOutput is internally, in somewhere, invoking resolveModuleNames...

cc @kitsonk since I don't really know things about the Language Service API...

(It feels like both TypeScript compiler and v8 ES modules are trying to gather all the dependencies, or at least recursively evaluating and resolving files for the import statements... ideally only one of them should happen

kitsonk · 2019-01-07T19:47:32Z

resolveModuleNames is the core language service API that TypeScript uses to understand the full type dependencies. TypeScript is going to build a dependency graph before it emits the first file, because it needs to have a holistic understanding. Once that is done, then I assume the ES Module logic is also building a dependency graph, as each module is actually getting emitted. When the Compiler and the Runner were both in user land, they would cache that, so that the Runner wouldn't ask again to fetch modules that were needed. It seems like #1460 needs to do the same thing and cache modules being fetched to be able to offer them up to TypeScript multiple times.

@kevinkassimo I would run the whole thing with the debug flag to validate that this is actually what is happening.

ry · 2019-01-09T22:53:13Z

@kevinkassimo could you please rebase?

kevinkassimo · 2019-01-10T04:44:32Z

@ry Rebased.

@kitsonk I'm pretty sure it is the problem.

Also days ago TimothyGu (former Node TSC who also worked on WHATWG standards) mentioned that browsers resolve modules when doing parsing and before actual module instantiation (before auto maybe_ok = module->InstantiateModule(context, ResolveCallback);), which I think is similar to what TypeScript compiler is actually doing here. Ideally only 1 dependency graph should be built...

ry · 2019-01-10T05:36:39Z

libdeno/binding.cc

@@ -530,7 +530,7 @@ void DenoIsolate::ResolveOk(const char* filename, const char* source) {
 }

 bool ExecuteMod(v8::Local<v8::Context> context, const char* js_filename,
-                const char* js_source) {
+                const char* js_source, int is_prefetch) {


Maybe the name of this argument should be changed at this level level of abstraction, because in libdeno we don't know about "prefetching" ...

What do you think about "resolve_only" or "without_execute"... something like that?

Also you may use a bool here as this is C++.

Adopting resolve_only (execute_mod that without_execute sounds weird...)

ry · 2019-01-10T05:37:11Z

libdeno/deno.h

@@ -76,7 +76,7 @@ int deno_execute(Deno* d, void* user_data, const char* js_filename,
 // Return value: 0 = fail, 1 = success
 // Get error text with deno_last_exception().
 int deno_execute_mod(Deno* d, void* user_data, const char* js_filename,
-                     const char* js_source);
+                     const char* js_source, int is_prefetch);


Same comment as above regarding the name of this argument

I am making them all bools now instead (C++, extern C and Rust)

ry · 2019-01-10T05:40:55Z

libdeno/libdeno_test.cc

@@ -294,7 +294,7 @@ TEST(LibDenoTest, ModuleResolutionFail) {
    // Do not call deno_resolve_ok();
  };
  Deno* d = deno_new(deno_config{0, empty, empty, nullptr, resolve_cb});
-  EXPECT_FALSE(deno_execute_mod(d, d, "a.js", mod_a));
+  EXPECT_FALSE(deno_execute_mod(d, d, "a.js", mod_a, 0));


It would be nice to have a libdeno test exercising the is_prefetch option (or whatever you end up naming it)

Suggest something like this (please change naming)

TEST(LibDenoTest, ModuleResolutionPrefetch) { static int count = 0; auto resolve_cb = [](void* user_data, const char* specifier, const char* referrer) { EXPECT_STREQ(specifier, "b.js"); EXPECT_STREQ(referrer, "a.js"); count++; auto d = reinterpret_cast<Deno*>(user_data); deno_resolve_ok(d, "b.js", mod_b); }; Deno* d = deno_new(deno_config{0, empty, empty, nullptr, resolve_cb}); EXPECT_TRUE(deno_execute_mod(d, d, "a.js", "import { retb } from 'b.js'\n" "throw Error('unreachable');" , 1)); EXPECT_EQ(count, 1); deno_delete(d); }

libdeno/libdeno_test.cc

ry

Nice! I'm happy to see how simply done this was.

The one problem that I have is that it doesn't have a test for the actual behavior - it seems it would need some custom bits of python.

kitsonk · 2019-01-10T05:44:18Z

@kevinkassimo I think it is a problem too and yes, browser do a two pass instantiation, it was something that we dealt with in the runner... In chatting with Ryan I am going to take a look at the whole thing and see if we can get things to behave a bit cleaner.

kevinkassimo · 2019-01-10T06:05:14Z

@ry Hmm the diff test currently at least proves that the files are downloaded (due to logs). Probably we should double check by walking into the deps/ folder to see if they are actually cached at the correct location?

kevinkassimo · 2019-01-10T06:15:03Z

@kitsonk Cool... Also, since we are having 2 independent dependency graphs without any communications at this moment, we might need to add on Rust/C++ side another function/map that resolves referrer and specifier to filenames (and possibly some metadata) and store the mappings, to make recompile/reload function correctly again...

ry · 2019-01-10T17:00:45Z

libdeno/deno.h

@@ -76,7 +76,7 @@ int deno_execute(Deno* d, void* user_data, const char* js_filename,
 // Return value: 0 = fail, 1 = success
 // Get error text with deno_last_exception().
 int deno_execute_mod(Deno* d, void* user_data, const char* js_filename,
-                     const char* js_source);
+                     const char* js_source, bool resolve_only);


Don't use bool here. deno.h is supposed to be ANSI C to support linking to Rust. Even if this isn't a problem currently, it is in Go, and we might be conservative.

Add a line of documentation regarding this parameter, please.

ry · 2019-01-10T17:38:31Z

@kevinkassimo I've added tools/prefetch_test.py please take a look

kevinkassimo · 2019-01-10T19:27:05Z

@ry The test looks good (although I'm investigating why it fails on Appveyor...)

(I actually just noticed that I used to have an integration test for --prefetch, but for some reason it was lost during rebasing... I'm adding them back)

ry · 2019-01-10T21:05:24Z

tests/prefetch.ts.out

+Downloading http://localhost:4545/tests/subdir/subdir3/mod1.ts...
+Downloading http://localhost:4545/tests/subdir/subdir3/mod2.ts...
+Downloading http://localhost:4545/tests/subdir/subdir3/mod3.ts...
+Downloading http://localhost:4545/tests/subdir/subdir3/mod1.ts...


Hrm. I guess this is #1496

They should have the same underlying problem...

ry · 2019-01-15T02:12:40Z

FYI we are looking into the windows failures here.

kevinkassimo · 2019-01-15T02:13:24Z

@ry Great... (annoying since I have no way of reproducing it on a non-Windows machine...)

piscisaureus · 2019-01-15T07:59:26Z

@ry, @kevinkassimo I added a commit that fixes windows.

ry

LGTM - thanks @kevinkassimo and @piscisaureus

kevinkassimo commented Jan 7, 2019

View reviewed changes

kevinkassimo force-pushed the esmodule/prefetch branch from 395b73e to 141b02f Compare January 10, 2019 04:40

ry reviewed Jan 10, 2019

View reviewed changes

libdeno/libdeno_test.cc Show resolved Hide resolved

ry reviewed Jan 10, 2019

View reviewed changes

kevinkassimo force-pushed the esmodule/prefetch branch from c68f2ce to d4ce9c8 Compare January 10, 2019 20:38

ry reviewed Jan 10, 2019

View reviewed changes

ry force-pushed the esmodule/prefetch branch 2 times, most recently from 3a4eeb1 to 1516135 Compare January 14, 2019 16:23

kevinkassimo and others added 6 commits January 15, 2019 08:20

Add --prefetch flag for deps prefetch without running

56b6657

Add libdeno test and param names

c5996ef

Add tools/prefetch_test.py

b617d1c

nit and integration test

95ecf9b

remove second prefetch test

89a8a03

adjust test

1febd93

ry added 2 commits January 15, 2019 08:20

adjust test

319e82d

use merge_env

2849c1e

piscisaureus force-pushed the esmodule/prefetch branch from b8e87d9 to f4e5759 Compare January 15, 2019 07:21

Fix windows

49c4759

piscisaureus force-pushed the esmodule/prefetch branch from f4e5759 to 49c4759 Compare January 15, 2019 07:23

ry approved these changes Jan 15, 2019

View reviewed changes

ry merged commit c870cf4 into denoland:master Jan 15, 2019

kevinkassimo deleted the esmodule/prefetch branch December 27, 2019 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --prefetch flag for deps prefetch without running #1475

Add --prefetch flag for deps prefetch without running #1475

kevinkassimo commented Jan 7, 2019 •

edited

Loading

kevinkassimo Jan 7, 2019

ry commented Jan 7, 2019

kevinkassimo commented Jan 7, 2019 •

edited

Loading

kitsonk commented Jan 7, 2019

ry commented Jan 9, 2019

kevinkassimo commented Jan 10, 2019

ry Jan 10, 2019

kevinkassimo Jan 10, 2019

ry Jan 10, 2019

kevinkassimo Jan 10, 2019

ry Jan 10, 2019

ry left a comment

kitsonk commented Jan 10, 2019

kevinkassimo commented Jan 10, 2019

kevinkassimo commented Jan 10, 2019 •

edited

Loading

ry Jan 10, 2019

ry commented Jan 10, 2019

kevinkassimo commented Jan 10, 2019

ry Jan 10, 2019

kevinkassimo Jan 10, 2019

ry commented Jan 15, 2019

kevinkassimo commented Jan 15, 2019

piscisaureus commented Jan 15, 2019

ry left a comment

Add --prefetch flag for deps prefetch without running #1475

Add --prefetch flag for deps prefetch without running #1475

Conversation

kevinkassimo commented Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

ry commented Jan 7, 2019

kevinkassimo commented Jan 7, 2019 • edited Loading

kitsonk commented Jan 7, 2019

ry commented Jan 9, 2019

kevinkassimo commented Jan 10, 2019

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

ry left a comment

Choose a reason for hiding this comment

kitsonk commented Jan 10, 2019

kevinkassimo commented Jan 10, 2019

kevinkassimo commented Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

ry commented Jan 10, 2019

kevinkassimo commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry commented Jan 15, 2019

kevinkassimo commented Jan 15, 2019

piscisaureus commented Jan 15, 2019

ry left a comment

Choose a reason for hiding this comment

kevinkassimo commented Jan 7, 2019 •

edited

Loading

kevinkassimo commented Jan 7, 2019 •

edited

Loading

kevinkassimo commented Jan 10, 2019 •

edited

Loading