Adds CodeCache handler #402

ry · 2018-07-24T06:33:42Z

Depends on #396

piscisaureus · 2018-07-24T14:21:12Z

build_extra/flatbuffers/rust/rust_flatbuffer.gni

@@ -0,0 +1,47 @@
+import("//build_extra/rust/rust.gni")
+
+# TODO(ry) "flatbuffer.gni" should be "flatbuffers.gni" we should be consistant


piscisaureus · 2018-07-24T14:22:08Z

build_extra/flatbuffers/rust/rust_flatbuffer.gni

+ "--rust",
+
+ #"--gen-mutable",
+ #"--keep-prefix",


Why are these commented out?

piscisaureus · 2018-07-24T14:24:46Z

src/deno_dir.rs

+// https://github.com/ry/deno/blob/golang/deno_dir.go#L99-L111
+pub fn setup() -> std::io::Result<()> {
+ // Only setup once.
+ if unsafe { ROOT != None } {


Why unsafe?

Because it's a static global.

piscisaureus · 2018-07-24T14:26:49Z

src/binding.cc

- v8::Uint8Array::New(ab, buf.data_ptr - buf.alloc_ptr, buf.data_len);
- return view;
+ if (buf.alloc_ptr == nullptr) {
+ // If alloc_ptr isn't set, we memcpy.


This is alright.

piscisaureus · 2018-07-24T18:24:22Z

src/deno_dir.rs

+ // In the Go code this was called SrcDir. Renaming to Deps.
+ // Example: /Users/rld/.deno/deps/
+ Deps,
+ // In the Go code this was called CacheDir. Renaming to GEN.


Gen or GEN ?

piscisaureus · 2018-07-24T18:24:52Z

src/deno_dir.rs

+}
+pub use self::Dirname::*;
+
+pub fn path(dirname: Dirname) -> &'static Path {


Using Path now, that's a step forward :)

piscisaureus · 2018-07-24T18:26:53Z

src/deno_dir.rs

+ if unsafe { ROOT != None } {
+ return Ok(());
+ }
+ let home_dir = std::env::home_dir().expect("Could not get home directory.");


Would rather convert home_dir Path/PathBuf here and get rid of the unsafe block below.

It is a PathBuf here already https://doc.rust-lang.org/std/env/fn.home_dir.html

The unsafe is for the static global assignment.

Ah, I didn't know this was considered unsafe

piscisaureus · 2018-07-24T18:30:32Z

src/deno_dir.rs

+
+// https://github.com/ry/deno/blob/golang/deno_dir.go#L32-L35
+pub fn cache_path(filename: &str, source_code: &str) -> PathBuf {
+ let cache_key = source_code_hash(filename, source_code);


So if a file's dependencies change, there's no need to recompile?
(Just asking, I don't know)

That's right. In the compiled code the requires will reference the original module specifier, not the specifier of the compiled file.

What I mean is, some file might need to be recompiled if it's dependencies change. Like in C++ when a header changes.
I'n typescript this may be the case too but maybe not, idk. Did you consider this?

@piscisaureus Yeah, I don't think that happens... but let's see if we find any examples as we flesh out the tests.

piscisaureus · 2018-07-24T18:31:32Z

src/fs.rs

+pub fn read_file_sync(path: &Path) -> std::io::Result<String> {
+ File::open(path).and_then(|mut f| {
+ let mut contents = String::new();
+ f.read_to_string(&mut contents)?;


This is not generally safe, because a file's contents are not necessarily valid utf-8.
We'll make it read to a buffer later? Otherwise consider renaming the function to read_text_file_sync.

Oh - I thought Strings were like std::string. Sigh. Will change...

This gonna stay as it is?

piscisaureus · 2018-07-24T18:40:56Z

src/handlers.rs

-fn string_from_ptr(ptr: *const c_char) -> String {
- let cstr = unsafe { CStr::from_ptr(ptr as *const i8) };
- String::from(cstr.to_str().unwrap())
+// Help. Is there a way to do this without macros?


I don't think so -- an &str expects to store a reference to it's underlying String, but *const c_char isn't and can't be used as such (a String is not just a pointer, but also stores length, capacity, refcount etc.).
This is the reason you have to construct a new String here.
If you make str_from_ptr a function then the String goes out of scope at the end of the function.
(you could also return both, but I suspect that might also defeat the purpose of this convenience wrapper).

piscisaureus · 2018-07-24T23:04:54Z

Mostly LGTM now. Strangely I didn manage to build this PR, but now that I'm trying again, I get this error (freshly checked out, cleaned, third_party reset and cleaned etc).

C:\Users\BertBelder\d\deno>python tools\build.py
C:\Users\BertBelder\d\deno\third_party\depot_tools\gn gen C:\Users\BertBelder\d\deno\out\debug
Traceback (most recent call last):
File "C:/Users/BertBelder/d/deno/build/vs_toolchain.py", line 16, in
from gn_helpers import ToGNString
ImportError: No module named gn_helpers
ERROR at //build/config/win/visual_studio_version.gni:27:7: Script returned non-zero exit code.
exec_script("../../vs_toolchain.py", [ "get_toolchain_dir" ], "scope")
^----------
Current dir: C:/Users/BertBelder/d/deno/out/debug/
Command: C:/Python27/python.exe -- C:/Users/BertBelder/d/deno/build/vs_toolchain.py get_toolchain_dir
Returned 1.
See //build/toolchain/win/BUILD.gn:8:1: whence it was imported.
import("//build/config/win/visual_studio_version.gni")
^----------------------------------------------------
See //BUILD.gn:9:1: which caused the file to be included.
group("all") {

piscisaureus · 2018-07-24T23:07:54Z

Hmm, it seems that git creates the wrong kind of symlinks now that third_party has become a separate repo.

robbym · 2018-07-26T01:46:41Z

Are the global mutables necessary? Can't all of this go into a struct that owns those globals and methods?

ry · 2018-07-26T14:21:43Z

@robbym I agree it shouldn't be global. I've reworked it to be a standalone struct.

ry · 2018-07-26T17:38:06Z

@piscisaureus PTAL. I've done some refactoring and added more tests.

DenoDir is now a standalone struct - instead of using global variables. This makes threaded testing possible and is also the right thing to do.
In order to store the DenoDir in the Deno struct and then retrieve it later, I've added from_c and deno_get_data.
Due to that remove_dir_all, you may find you don't have all the deps for windows. I didn't want to inadvertently add unused stuff.. so I'll leave it to you to add winapi, if it is needed.

This does indeed give us 10x startup on a warm run:

~/src/deno> rm -rf ~/.deno/
~/src/deno> time ./out/release/deno tests/002_hello.ts
Hello World

real	0m0.615s
user	0m0.965s
sys	0m0.066s
~/src/deno> time ./out/release/deno tests/002_hello.ts
Hello World

real	0m0.074s
user	0m0.036s
sys	0m0.032s
~/src/deno>

piscisaureus · 2018-07-28T16:53:12Z

The test fails for me:

output

running 9 tests test deno_dir::test_resolve_module ... ok test deno_dir::test_cache_path ... ok test deno_dir::test_source_code_hash ... ok test deno_dir::test_code_cache ... ok test deno_dir::test_src_file_to_url ... ok test test_parse_core_args_1 ... ok test test_parse_core_args_2 ... ok test test_c_to_rust ... ok test deno_dir::test_code_fetch ... FAILED

failures:

---- deno_dir::test_code_fetch stdout ----
thread 'deno_dir::test_code_fetch' panicked at 'called Result::unwrap() on an Err value: ()', libcore\result.rs:945:5
note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.
stack backtrace:
0: std::sys::windows::backtrace::unwind_backtrace
at C:\projects\rust\src\libstd\sys\windows\backtrace\mod.rs:65
1: std::sys_common::backtrace::print
at C:\projects\rust\src\libstd\sys_common\backtrace.rs:59
2: std::panicking::default_hook::{{closure}}
at C:\projects\rust\src\libstd\panicking.rs:211
3: std::panicking::default_hook
at C:\projects\rust\src\libstd\panicking.rs:221
4: std::panicking::rust_panic_with_hook
at C:\projects\rust\src\libstd\panicking.rs:463
5: std::panicking::begin_panic_fmt
at C:\projects\rust\src\libstd\panicking.rs:350
6: std::panicking::rust_begin_panic
at C:\projects\rust\src\libstd\panicking.rs:328
7: core::panicking::panic_fmt
at C:\projects\rust\src\libcore\panicking.rs:71
8: core::result::unwrap_failed<()>
at C:\projects\rust\src\libcore\macros.rs:26
9: core::result::Result<url::Url, ()>::unwrapurl::Url,()
at C:\projects\rust\src\libcore\result.rs:782
10: test_rs_bin::deno_dir::DenoDir::resolve_module
at .\src\deno_dir.rs:169
11: test_rs_bin::deno_dir::DenoDir::code_fetch
at .\src\deno_dir.rs:98
12: test_rs_bin::deno_dir::test_code_fetch
at .\src\deno_dir.rs:273
13: test_rs_bin::__test::TESTS::{{closure}}
at .\src\deno_dir.rs:264
14: core::ops::function::FnOnce::call_once<closure,()>
at C:\projects\rust\src\libcore\ops\function.rs:223
15: alloc::boxed::{{impl}}::call_box<(),closure>
at C:\projects\rust\src\liballoc\boxed.rs:638
16: panic_unwind::__rust_maybe_catch_panic
at C:\projects\rust\src\libpanic_unwind\lib.rs:105

failures:
deno_dir::test_code_fetch

test result: FAILED. 8 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

The issue is that Url::from_file_path(&containing_file).unwrap(); panics if containing_file isn't an absolute path, and /baddir/badfile.ts isn't absolute on my system.

ry · 2018-07-28T18:15:02Z

@piscisaureus I believe I've fixed that using your add_root! macro.

piscisaureus

Your last patch makes the tests pass on windows.
The startup time numbers are really encouraging. Nice work!

I wonder what the "fix logger" commit fixes - I never had any trouble with it?

I had a few comments. See what you wanna do with them, lgtm otherwise.

piscisaureus · 2018-07-29T03:44:24Z

src/deno_dir.rs

+ if cache_path.exists() {
+ Ok(())
+ } else {
+ let mut file = File::create(cache_path)?;


This is a race condition w.r.t to exists() -- probably should create the file in exclusive mode.
I also worry what might happen is there are two processes and one reads the cache file while the other is in the midst of writing it.
Not suggesting you fix it as part of this PR, but it'll have to be done some time.

I added a todo comment.

piscisaureus · 2018-07-29T03:46:53Z

src/fs.rs

+pub fn read_file_sync(path: &Path) -> std::io::Result<String> {
+ File::open(path).and_then(|mut f| {
+ let mut contents = String::new();
+ f.read_to_string(&mut contents)?;


This gonna stay as it is?

piscisaureus · 2018-07-29T03:52:06Z

src/handlers.rs

+ let errmsg = format!("{}", err);
+ reply_error(d, cmd_id, &errmsg);
+ }
+ // null response indicates success.


Implicit null response is a foot gun. Probably better to explicitly say reply_null() if a handler succeeds without a return value.

Agree. I think this whole interface is going to change dramatically soon. This is just a first pass. I'll leave it as is now.

piscisaureus · 2018-07-29T03:54:58Z

src/main.rs

+ ptr: 0 as *const binding::DenoC,
+ dir: deno_dir::DenoDir::new(None).unwrap(),
+ });
+ let deno: &'a mut Deno = Box::leak(deno_box);


Jeez... I can see how this makes for some sleepless nights.
Why is Deno made mutable here if it gets cast to *const one line below?
(Not saying it's wrong, I just don't understand this very well.)

A little comment on what's going on here would be in order too (as in -- mention that C-Deno needs a reference to Rust-Deno. And why this leak is necessary (is it really btw?).

It gets returned as mut (but my confidence that this is optimal code is low)

This is a utility function for CodeCache and other handlers.

Fixes: denoland#402

They finally tagged v2.0.0 so there's no reason to use our own fork any longer.

…enoland#443) A little cleanup and showcase how to wire up custom module types in the `fs_module_loader.rs` example. Ref denoland#402

ry force-pushed the code_cache branch 2 times, most recently from b4ef3fb to 10ded40 Compare July 24, 2018 17:04

piscisaureus reviewed Jul 24, 2018

View reviewed changes

ry force-pushed the code_cache branch from 1023b1c to 2808790 Compare July 24, 2018 19:39

ry force-pushed the code_cache branch from 2808790 to b110a51 Compare July 26, 2018 14:20

ry force-pushed the code_cache branch 2 times, most recently from 8c27dcc to 5bf369b Compare July 26, 2018 17:34

ry force-pushed the code_cache branch 3 times, most recently from 148cc38 to 6e47cfc Compare July 26, 2018 22:01

piscisaureus mentioned this pull request Jul 28, 2018

Add winapi crate #424

Closed

ry force-pushed the code_cache branch from 9796ecd to 6f657cf Compare July 28, 2018 18:14

piscisaureus approved these changes Jul 29, 2018

View reviewed changes

ry and others added 8 commits July 29, 2018 00:05

Add tests/003_relative_import.ts

dcf94da

Implement a logger so we can get logging output.

5ffc67d

Add from_c() to get a Deno object from ptr.

4546803

This is a utility function for CodeCache and other handlers.

spelling

ff1932d

Add sha1 and tempfile crates.

eb47067

Implement CodeCache

94dc58c

Add "features" argument to rust components

e2b4081

Add winapi crate, make 'rand' and 'tempfile' work

5043fd8

ry force-pushed the code_cache branch from 6f657cf to 5043fd8 Compare July 29, 2018 04:12

ry merged commit 604a8a6 into master Jul 29, 2018

ry deleted the code_cache branch July 29, 2018 04:22

piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019

http: send an empty response body if none is provided (denoland#429)

e00e3fe

Fixes: denoland#402

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Jun 5, 2020

ci: use upstream actions/cache@v2 (denoland#402)

3f47d69

They finally tagged v2.0.0 so there's no reason to use our own fork any longer.

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Jun 5, 2020

ci: upgrade to Rust 1.44.0 (denoland#402)

cbc8556

		@@ -0,0 +1,47 @@
		import("//build_extra/rust/rust.gni")

		# TODO(ry) "flatbuffer.gni" should be "flatbuffers.gni" we should be consistant

Adds CodeCache handler #402

Adds CodeCache handler #402

Conversation

ry commented Jul 24, 2018

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

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

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

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

piscisaureus commented Jul 24, 2018

piscisaureus commented Jul 24, 2018

robbym commented Jul 26, 2018

ry commented Jul 26, 2018

ry commented Jul 26, 2018 • edited Loading

piscisaureus commented Jul 28, 2018 • edited Loading

ry commented Jul 28, 2018

piscisaureus left a 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

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 commented Jul 26, 2018 •

edited

Loading

piscisaureus commented Jul 28, 2018 •

edited

Loading