Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Native ES modules #1460

Merged
merged 2 commits into from
Jan 9, 2019
Merged

Native ES modules #1460

merged 2 commits into from
Jan 9, 2019

Conversation

ry
Copy link
Member

@ry ry commented Jan 4, 2019 •
edited
Loading

Major refactor of internal compiler.

Before: JS and TS both were sent through the typescript compiler where their imports were parsed and handled. Both compiled to AMD JS and finally sent to V8

Now: JS is sent directly into V8. TS is sent through the typescript compiler, but tsc generates ES modules now instead of AMD. This generated JS is then dumped into V8.

This should much faster for pure JS code. It may improve TS compilation speed.

In the future this allows us to separate TS out of the runtime heap and into its own dedicated snapshot. This will result in a smaller runtime heap, and thus should be faster.

Many tests had to be disabled to make this happen. Apologies - but as you can see this is a very complex patch - and I had to sledge hammer it into place in a few places.

Also worth noting that this is necessary to support WASM

Fixes #975
Fixes #1190

Preliminary PRs:

ry
ry commented Jan 4, 2019
View reviewed changes
js/text_encoding_test.ts
@@ -65,17 +54,6 @@ test(function textDecoderErrorEncoding() {
assert(didThrow);
});

test(function textEncoder() {
const fixture = "������";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely due to ea6c9f2 .. i regret landing it now because I think it was actually incorrect. will attempt to revert the revert and see if this is fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm... this particular string is specifically designed to test the upper limit of high-surrogate pairs. If there was some sort of encoding issue, injecting mis-encoded stuff from Rust could cause this issue. I double checked just now in Chrome and the test is correct in that the encoding of the bytes of the string is right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya it’s working as intended and breaking something :) not sure what it is tho ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to keep these tests (unless there's actually something wrong with them).

ry
ry commented Jan 4, 2019
View reviewed changes
js/unit_tests.ts
@@ -6,7 +6,7 @@
import "./blob_test.ts";
import "./buffer_test.ts";
import "./chmod_test.ts";
import "./compiler_test.ts";
// import "./compiler_test.ts";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to try to get compiler_test working in this PR. It's too much of a headache. We will need to do several passes of refactoring on compiler after this PR is landed.

This was referenced Jan 4, 2019
Closed
Closed
@ry ry mentioned this pull request Jan 4, 2019
Closed
@ry ry force-pushed the esm branch 5 times, most recently from fd93825 to fd4b392 Compare January 4, 2019 17:22
This was referenced Jan 4, 2019
Merged
Merged
kevinkassimo
kevinkassimo reviewed Jan 4, 2019
View reviewed changes
js/compiler.ts Outdated Show resolved Hide resolved
@kevinkassimo kevinkassimo mentioned this pull request Jan 7, 2019
Merged
@ry ry mentioned this pull request Jan 7, 2019
Merged
@ry ry force-pushed the esm branch 2 times, most recently from 4735a30 to 6fe1a69 Compare January 8, 2019 21:28
@ry
Native ES modules
5b9dc7f 
This is a major refactor of internal compiler.

Before: JS and TS both were sent through the typescript compiler where
their imports were parsed and handled. Both compiled to AMD JS and
finally sent to V8

Now: JS is sent directly into V8. TS is sent through the typescript
compiler, but tsc generates ES modules now instead of AMD. This
generated JS is then dumped into V8.

This should much faster for pure JS code. It may improve TS compilation
speed.

In the future this allows us to separate TS out of the runtime heap and
into its own dedicated snapshot. This will result in a smaller runtime
heap, and thus should be faster.

Many tests had to be disabled to make this happen. Apologies - but as
you can see this is a very complex patch - and I had to sledge hammer it
into place in a few places.

Also worth noting that this is necessary to support WASM
piscisaureus
piscisaureus reviewed Jan 9, 2019
View reviewed changes
js/compiler.ts Outdated Show resolved Hide resolved
piscisaureus
piscisaureus reviewed Jan 9, 2019
View reviewed changes
js/text_encoding_test.ts
@@ -25,17 +25,6 @@ test(function btoaFailed() {
assertEqual(err.name, "InvalidInput");
});

test(function textDecoder() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this test need to go? It no longer passes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this file isn't valid UTF-8, it dies on v8::String::NewFromUtf8... I'm not sure how it worked before. I will investigate...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently code is loaded through the CodeFetch op. The source code is passed through the flatbuffer message and decoded by the flatbuffer library on the JS side. Looking at that source code, it does not die on invalid utf-8 input:
https://github.com/denoland/deno_third_party/blob/de4fbe4edfdabfbd053a0cca1bb113cfc97da73b/node_modules/flatbuffers/js/flatbuffers.js#L1082-L1138

With this change, source code is loaded more directly in C++ using v8::String::NewFromUtf8, which appear to be stricter.

There's no obvious solution to this.... I could run the source code through some sort of sanitizer first?

Copy link
Member Author

@ry ry Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will land with the tests removed. The file isn't valid utf8 so its of questionable validity.

That said, I think we ought to sanitize input and accept invalid utf8 - but it's a much more minor concern than this refactor.

I've added these issues to track:
#1488
#1489

piscisaureus
piscisaureus reviewed Jan 9, 2019
View reviewed changes
src/resources.rs Outdated Show resolved Hide resolved
piscisaureus
piscisaureus approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see lots of loose ends but nothing that looks like an oversight on your part.
Please save the encoding tests tho...

I'm going to approve this now so you you're not going to be held up.
But don't throw away the encoding tests pls...

@ry ry merged commit 0ceb554 into denoland:master Jan 9, 2019
@shreeve shreeve mentioned this pull request Jan 9, 2019
Closed
@bartlomieju bartlomieju mentioned this pull request Feb 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinkassimo kevinkassimo kevinkassimo left review comments

@kitsonk kitsonk kitsonk left review comments

@piscisaureus piscisaureus piscisaureus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ry @piscisaureus @kitsonk @kevinkassimo