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

refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner #71

Merged
merged 8 commits into from
Jul 11, 2022

Conversation

zong-zhe
Copy link
Contributor

@zong-zhe zong-zhe commented Jun 2, 2022

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

fix #67 and #82

2. What is the scope of this PR (e.g. component or file name):

kclvm-runner/runner.rs
kclvm-runner/lib.rs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other
  1. Encapsulate dylibs generating and executing in kclvm/lib.rs into kclvm-runner.
  2. Add struct "KclvmAssembler" in kclvm-runner/runner.rs to provide method "gen_libs" for dynamic link library generating.
  3. Add struct "KclvmLinker" in kclvm-runner/runner.rs to provide method "link_all_dylibs" for dynamic link library linking.
  4. Add method "execute" in kclvm-runner/lib.rs to encapsulate dylibs generating(gen_libs), dynamic link library linking(link_all_libs) and running(runner.run) together.
  5. The main purpose of separating the three parts from kclvm/lib.rs and encapsulating them into kclvm-runner is to support reuse in kcl-vet.
  6. Decoupling the assembler and llvm.
  7. The assembling LLVM IR into a dynamic link library is encapsulated into "LlvmLibAssembler" separately.
  8. Add trait "LibAssembler" to describe the interface "KclvmLibAssembler" should have. If other intermediate code is added in the future, the corresponding assembler must implement this trait.
  9. Struct "LlvmLibAssembler" is provided in "KclvmLibAssembler". "KclvmLibAssembler" is an enum that implements trait "LibAssembler". "KclvmLibAssembler" is responsible for the compilation of a single kcl file in one thread.
  10. "KclvmAssembler" is responsible for the concurrent compilation of multiple kcl files.
  11. "KclvmAssembler" will call the method in "KclvmLibAssembler" to generate a dynamic link library for a single kcl file in each thread of concurrent compilation.

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • Other

Add test cases in kclvm-runner/src/tests.rs and kclvm-runner/test_datas.
Add bench in kclvm-runner/benches/bench_runner.rs

Benchmark stats
1. KCL Test Case
// kclvm/runner/src/test_datas/init_check_order_0/main.k
schema Person:
    name: str
    age: int
    gender: str
    info: str = "{}, {}, {} years old".format(name, gender, age)

alice = Person {
    "name": "alice",
    "age": 10,
    "gender": "female"
}
2. Bench Stats (Compare with before refactoring)
2.1 CLI Report

image

2.2 HTML Report

image
image

6. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@zong-zhe zong-zhe requested review from chai2010 and Peefy June 2, 2022 04:33
@Peefy Peefy added enhancement New feature or request refactor labels Jun 2, 2022
@Peefy Peefy added this to the v0.4.3 Release milestone Jun 2, 2022
@Peefy Peefy added the tool Issues or PRs related to kcl tools inlucding format, lint, validation, document tools, etc. label Jun 2, 2022
kclvm/runner/src/runner.rs Outdated Show resolved Hide resolved
kclvm/runner/src/lib.rs Show resolved Hide resolved
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from 4e0c496 to e86954b Compare June 6, 2022 01:08
…cuting in kclvm/lib.rs into kclvm-runner

The assembler and linker of the current version of the compiler are not separately packaged.
In order to support the reuse of modules such as dylibs generating,linking and executing,
This modification separates dylibs generating,and encapsulate them individually into KclvmAssembler and KclvmLinker.

Encapsulate dylibs generating, linking and executing into kclvm-runner/assembler.rs and kclvm-runner/linker.rs.
Add struct "KclvmAssembler" in kclvm-runner/assembler.rs to provide method "gen_dylibs" for dylibs generating.
Add struct "KclvmLinker" in kclvm-runner/linker.rs to provide method "link_all_dylibs" for dylib linking.
Add method "execute" in kclvm-runner/lib.rs to encapsulate dylibs generating(gen_dylib), dylib linking(link_all_dylib) and running(runner.run) together.

fix #67
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from e86954b to f8e2142 Compare June 6, 2022 01:45
kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
kclvm/runner/src/lib.rs Show resolved Hide resolved
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch 2 times, most recently from fa3b156 to 18b5366 Compare June 6, 2022 04:10
@ldxdl
Copy link
Contributor

ldxdl commented Jun 6, 2022

Pls put benchmark stats into the PR info.

@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from 18b5366 to 4c272db Compare June 6, 2022 06:12
Copy link
Contributor

@ldxdl ldxdl left a comment

Choose a reason for hiding this comment

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

As a refactor PR, it would be good to have some preparation for future backend target abstractions.

kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from 4c272db to 99da07b Compare June 6, 2022 07:20
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from af8d1c9 to dfc6b70 Compare June 7, 2022 09:00
@zong-zhe zong-zhe requested review from Peefy and ldxdl June 7, 2022 10:25
kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
@ldxdl
Copy link
Contributor

ldxdl commented Jun 8, 2022

Do not hardcode 'dylib' in all naming. In fact, it may be .ddl, .dylib, .so on different platforms. I prefer to change all 'dylib's to 'lib's.

kclvm/runner/src/assembler.rs Outdated Show resolved Hide resolved
@ldxdl
Copy link
Contributor

ldxdl commented Jun 8, 2022

This refactoring should preferably do some preparation for the subsequent target abstraction refactoring. For example, dylib and llvm were all hardcoded as the only impl way, you may need to change them into variables first, and then the next refactoring will replace these variables through traits or other dynamic methods to make it a target abstraction with a set of impls. At the same time, do not hardcode implementation keywords like 'dylib', 'llvm' in the comments and docstrings.

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

You'd better lint code by using cargo clippy in the runner crate.

@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from dfc6b70 to 838f474 Compare June 10, 2022 10:40
@zong-zhe zong-zhe requested a review from ldxdl June 10, 2022 10:43
@Peefy Peefy force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from dcb5ab1 to 0fe6ac2 Compare June 20, 2022 09:38
@Peefy Peefy changed the title WIP refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner Jun 20, 2022
@Peefy Peefy force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch 2 times, most recently from 8b762e4 to 2c21d67 Compare June 24, 2022 05:24
@Peefy Peefy force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from 2c21d67 to efe821a Compare June 24, 2022 12:10
@Peefy Peefy changed the title refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner WIP refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner Jun 30, 2022
@Peefy Peefy changed the title WIP refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner [WIP] refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner Jun 30, 2022
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from 958cb57 to c589e87 Compare July 4, 2022 02:20
@kcl-lang kcl-lang deleted a comment from github-actions bot Jul 4, 2022
@Peefy Peefy changed the title [WIP] refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner refactor(kclvm-runner): encapsulate dylib generating, linking and executing in kclvm/lib.rs into kclvm-runner Jul 7, 2022
kclvm/runner/Cargo.toml Outdated Show resolved Hide resolved
internal/kclvm_py/scripts/update-kclvm.sh Outdated Show resolved Hide resolved
kclvm/runner/src/lib.rs Outdated Show resolved Hide resolved
kclvm/runner/src/lib.rs Outdated Show resolved Hide resolved
kclvm/runner/src/lib.rs Outdated Show resolved Hide resolved
kclvm/src/main.rs Show resolved Hide resolved
@zong-zhe zong-zhe requested a review from Peefy July 8, 2022 02:31
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from 597789b to debf405 Compare July 8, 2022 07:30
Peefy
Peefy previously approved these changes Jul 11, 2022
Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

…ting into kclvm-runner.

1. Encapsulate generating of libs  in "kclvm/src/lib.rs" and "kclvm/src/main.rs"  into "kclvm/runner/assembler.rs".
2. Encapsulate linking of libs in "kclvm/src/lib.rs" and "kclvm/src/main.rs" into "kclvm/runner/linker.rs"
3. Encapsulate executing of libs  in "kclvm/src/lib.rs" and "kclvm/src/main.rs" into "kclvm/runner/lib.rs".
4. A timer is added during the concurrent multi-file compilation to prevent KCLVM locked due to child thread panic.

fix #67 #106 #82
@zong-zhe zong-zhe force-pushed the refactor/zong-zhe/add_eval_to_kclvm_runner branch from debf405 to 6409a6d Compare July 11, 2022 10:17
Copy link
Contributor

@chai2010 chai2010 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

@Peefy Peefy merged commit 6f7ec98 into main Jul 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2022
@zong-zhe zong-zhe linked an issue Jul 11, 2022 that may be closed by this pull request
@Peefy Peefy deleted the refactor/zong-zhe/add_eval_to_kclvm_runner branch July 13, 2022 02:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request refactor tool Issues or PRs related to kcl tools inlucding format, lint, validation, document tools, etc.
Projects
None yet
5 participants