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

Support for native compilers #87

Merged
merged 33 commits into from
Feb 1, 2022
Merged

Support for native compilers #87

merged 33 commits into from
Feb 1, 2022

Conversation

cd1m0
Copy link
Contributor

@cd1m0 cd1m0 commented Jan 10, 2022

This is a WIP PR for collaborating on implementing support for using the native compiler binaries instead of the wasm ones.
WASM binaries sometimes take a lot more time (likely loading or first JIT-ing). This causes us to have VERY long test cycles, with CI often taking an hour.

This PR has several things implemented already:

  1. Adds a new option "frontend" to all the compile entry points. Valid values are CompilerFrontend.{Default, WASM, Native}. (Note that the name 'CompilerFrotend' is terrible. We should pick something more accurate. CompilerBackend maybe? Idk)

  2. Re-factors a lot of the compiling logic into new classes in frontend/wasm.ts and native_compilers.ts (note - native_compilers.ts should be moved under frontend)

  3. Makes all of the compile* entry-points async (as they may need to fetch a native compiler on-the-fly)

  4. Adds logic to download compiler binaries for the current architecture in native_compilers.ts

  5. Adds a new partial Soliidty parser for top-level definitions only in compile/inference/top_level_definitions_parser.pejgs. We can use this to detect all import statement in a file (along with all compiler pragmas as well)

  6. Adds some unit testing for the new parser

And its missing several more things:

  1. frontend/CompilerFrontend is the wrong name - should probably be more backend
  2. Add a new unit test that does the folowing:

for every solidity file X.sol in the samples, that doesn't import anything:
let a = top level definitions in X.sol after running the WASM frontend
let b = top level definitions in all files discovered after running the top-level definitions parser on X.sol
expect(a).toEqual(b);

  1. Run the unit tests from (4) on a sizable subset of flattened files from the contract archive repo (to sanity check the parser)

  2. Write the logic in the native_compilers.ts that uses the top-level definitions parser, and the ImportFinder provided by the callers, to find all imported files by the compiler. If any of those files live require a path-remapping (e.g. @open-zeppelin imported from node_modules/), then build a proper path-remapping option to be passed to the native compiler

  3. Add a unit test that runs both the native compiler and the wasm compiler for every sample that we have under tests, asserting that they produce the same ASTs

  4. Run test 7 for a good subset of the contract archive repo to sanity-check the whole logic.

I think at this point we can trust the code enough that it works. @blitz-1306 what do you think?

@blitz-1306 blitz-1306 self-requested a review January 10, 2022 10:18
@blitz-1306 blitz-1306 added breaking change Changes that would cause a backward compatibility break enhancement New feature or request labels Jan 10, 2022
Copy link
Contributor

@blitz-1306 blitz-1306 left a comment

Choose a reason for hiding this comment

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

Tremendous amount of thoughts and ideas! I realized that it is still WIP, still I would contribute some review findings, that would help polish the functionlity. I would take some of them for myself to address further.

src/ast/constants.ts Outdated Show resolved Hide resolved
src/ast/constants.ts Outdated Show resolved Hide resolved
src/ast/constants.ts Outdated Show resolved Hide resolved
src/bin/compile.ts Outdated Show resolved Hide resolved
src/bin/compile.ts Outdated Show resolved Hide resolved
src/compile/frontends/native_compilers.ts Outdated Show resolved Hide resolved
src/compile/frontends/wasm.ts Outdated Show resolved Hide resolved
src/compile/frontends/wasm.ts Outdated Show resolved Hide resolved
src/compile/inference/top_level_definitions.pegjs Outdated Show resolved Hide resolved
src/compile/utils.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022

Codecov Report

Merging #87 (38b4d61) into master (0de0365) will increase coverage by 0.03%.
The diff coverage is 87.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   92.23%   92.26%   +0.03%     
==========================================
  Files         246      254       +8     
  Lines        5033     5195     +162     
  Branches      800      817      +17     
==========================================
+ Hits         4642     4793     +151     
- Misses        249      253       +4     
- Partials      142      149       +7     
Impacted Files Coverage Δ
src/compile/import_resolver.ts 90.00% <ø> (-3.11%) ⬇️
src/types/abi.ts 80.30% <50.00%> (ø)
src/compile/kinds/native.ts 73.46% <73.46%> (ø)
src/compile/kinds/md.ts 73.52% <73.52%> (ø)
src/compile/input.ts 78.57% <78.57%> (ø)
src/bin/compile.ts 89.33% <88.57%> (-2.10%) ⬇️
src/compile/utils.ts 91.45% <93.10%> (+0.86%) ⬆️
src/compile/inference/imports.ts 96.07% <96.07%> (ø)
src/compile/kinds/wasm.ts 96.87% <96.87%> (ø)
src/compile/compiler_selection.ts 83.33% <100.00%> (+0.83%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0de0365...38b4d61. Read the comment docs.

@blitz-1306 blitz-1306 changed the title [WIP] Native compilers Support for native compilers Jan 20, 2022
@blitz-1306 blitz-1306 self-requested a review January 20, 2022 20:27
Copy link
Contributor

@blitz-1306 blitz-1306 left a comment

Choose a reason for hiding this comment

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

Looking more like "final", except few TODOs that are still need to be addressed (compiler cache location, native compiler list updates).

@blitz-1306 blitz-1306 self-requested a review January 24, 2022 10:09
blitz-1306
blitz-1306 previously approved these changes Jan 24, 2022
Copy link
Contributor Author

@cd1m0 cd1m0 left a comment

Choose a reason for hiding this comment

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

Awesome work on expanding the tests and cleanin up Pavel!

Found a couple of small bugs and missing docstrings in my code.
Also made some general comments on improving some tests in later PRs.

Will fix my comments, file issues for anything in later PRs and then re-run tests. Other than that I am good with it too.

src/compile/inference/imports.ts Show resolved Hide resolved
src/compile/inference/imports.ts Show resolved Hide resolved
src/compile/inference/imports.ts Show resolved Hide resolved
src/compile/inference/imports.ts Show resolved Hide resolved
src/compile/inference/imports.ts Outdated Show resolved Hide resolved
src/compile/utils.ts Outdated Show resolved Hide resolved
test/integration/compile/kinds.spec.ts Outdated Show resolved Hide resolved
test/integration/factory/copy.spec.ts Show resolved Hide resolved
test/integration/sourcemap/snapshot.spec.ts Show resolved Hide resolved
test/utils/file.ts Outdated Show resolved Hide resolved
src/compile/kinds/native.ts Outdated Show resolved Hide resolved
@cd1m0 cd1m0 mentioned this pull request Jan 28, 2022
blitz-1306
blitz-1306 previously approved these changes Jan 28, 2022
@blitz-1306 blitz-1306 merged commit b95b610 into master Feb 1, 2022
@blitz-1306 blitz-1306 deleted the native_compilers branch February 1, 2022 09:14
@blitz-1306 blitz-1306 mentioned this pull request Feb 2, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that would cause a backward compatibility break enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants