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.

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

Rationalise compiler ops #1740

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Rationalise compiler ops #1740

merged 1 commit into from
Feb 18, 2019

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Feb 11, 2019

This PR removes the code cache OP from the runtime and rationalises the response from the compiler. Currently, the compiler performs a cache op and then returns the full module (including the source code) back to Rust. Rust already knows the source code and knows it needs to try to cache the result after a compile, so it handles all of this, removing the need for the compiler to perform these actions.

Because I am still a Rust infant, I am sure I made mistakes in the Rust code. I still need to restore the tests and write new ones.

I made the decision to rename CodeFetchOutput to ModuleMetaData because that seem better to represent what is happening, as the object doesn't always come from a code fetch and is incrementally enhanced after the compile.

@kitsonk kitsonk force-pushed the compiler-nocache branch 2 times, most recently from 26b0a04 to d586dfe Compare February 11, 2019 06:10
@kitsonk kitsonk changed the title [WIP] Rationalise compiler ops Rationalise compiler ops Feb 11, 2019
ry
ry previously approved these changes Feb 11, 2019
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

I'm slightly worried we may want CodeCache in the compiler isolate again at some point in the future - but I think it's good practice to remove dead code.

src/compiler.rs Show resolved Hide resolved
src/compiler.rs Outdated Show resolved Hide resolved
@ry ry dismissed their stale review February 15, 2019 15:35

waiting for signature change on compile_sync

@kitsonk
Copy link
Contributor Author

kitsonk commented Feb 18, 2019

Ok, the PR now does the following:

  • Moves the caching of the compiled output to Rust from compiler runtime. Removes the code_cache op. The compiler was sending back all the data anyways in compile_async, so it was effectively doing it twice.
  • Converts the code_fetch op to fetch_module_meta_data and moves the source code from a string to an 8-Bit array.
  • Generally migrates source code, output code and source maps from String to Vec<u8> within Rust.
  • Tries to make the term ModuleMetaData consistent for objects that represent a bundle of a modules descriptor/source code/compiled code/source maps, be it in Rust or the compiler runtime.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks Kitson !

@ry ry merged commit 54094c7 into denoland:master Feb 18, 2019
@kitsonk kitsonk deleted the compiler-nocache branch June 6, 2019 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants