From 0fcc2b79deb741fb5b68abdcba8e155a4d2f30d2 Mon Sep 17 00:00:00 2001 From: zongz <68977949+zong-zhe@users.noreply.github.com> Date: Fri, 23 Sep 2022 19:50:06 +0800 Subject: [PATCH] Fix(kclvm-runner): Fixed file locks in kclvm-runner. (#215) * Replace relative paths with absolute paths in test cases. * Fix(kclvm-runner): Fixed file locks in kclvm-runner. The operations of the read and write cache are also wrapped in the file lock. fix issue #213. * remove useless println! * modified method `clean_path` from pub to pub(crate) --- kclvm/Cargo.lock | 1 + kclvm/runner/Cargo.toml | 1 + kclvm/runner/src/assembler.rs | 169 +++++++++++++--------------------- kclvm/runner/src/lib.rs | 7 +- kclvm/runner/src/tests.rs | 114 +++++++++++++---------- kclvm/sema/src/ty/into.rs | 2 +- 6 files changed, 139 insertions(+), 155 deletions(-) diff --git a/kclvm/Cargo.lock b/kclvm/Cargo.lock index 4b47a8590..0268c5e69 100644 --- a/kclvm/Cargo.lock +++ b/kclvm/Cargo.lock @@ -741,6 +741,7 @@ dependencies = [ name = "kclvm-runner" version = "0.1.0" dependencies = [ + "anyhow", "chrono", "criterion", "fslock", diff --git a/kclvm/runner/Cargo.toml b/kclvm/runner/Cargo.toml index 021a22cac..501819ca5 100644 --- a/kclvm/runner/Cargo.toml +++ b/kclvm/runner/Cargo.toml @@ -17,6 +17,7 @@ libloading = "0.7.3" threadpool = "1.0" chrono = "0.4.19" tempfile = "3.3.0" +anyhow = "1.0" kclvm-ast = {path = "../ast", version = "0.1.0"} kclvm-parser = {path = "../parser", version = "0.1.0"} diff --git a/kclvm/runner/src/assembler.rs b/kclvm/runner/src/assembler.rs index fb412ed24..d7c8ea78a 100644 --- a/kclvm/runner/src/assembler.rs +++ b/kclvm/runner/src/assembler.rs @@ -69,56 +69,10 @@ pub(crate) trait LibAssembler { lib_path: &str, ) -> String; - /// This method is prepared for concurrent compilation in KclvmAssembler. - /// It is an atomic method executed by each thread in concurrent compilation. - /// - /// This method will take the above method “assemble_lib” as a hook method to - /// generate the dynamic link library, and lock the file before calling “assemble_lib”, - /// unlocked after the call ends, - #[inline] - fn lock_file_and_gen_lib( - &self, - compile_prog: &Program, - import_names: IndexMap>, - file: &Path, - ) -> String { - let code_file = file.to_str().unwrap(); - let code_file_path = &self.add_code_file_suffix(code_file); - let lock_file_path = &format!("{}.lock", code_file_path); - let lib_path = format!("{}{}", code_file, Command::get_lib_suffix()); - - // Locking file for parallel code generation. - let mut file_lock = - fslock::LockFile::open(lock_file_path).expect(&format!("{} not found", lock_file_path)); - file_lock.lock().unwrap(); - - // Calling the hook method will generate the corresponding intermediate code - // according to the implementation of method "assemble_lib". - let gen_lib_path = self.assemble_lib( - compile_prog, - import_names, - code_file, - code_file_path, - &lib_path, - ); - - // Unlock file - file_lock.unlock().unwrap(); - - gen_lib_path - } - - #[inline] - fn clean_path(&self, path: &str) { - if Path::new(path).exists() { - std::fs::remove_file(&path).unwrap(); - } - } - #[inline] fn clean_lock_file(&self, path: &str) { let lock_path = &format!("{}.lock", self.add_code_file_suffix(path)); - self.clean_path(lock_path); + clean_path(lock_path); } } @@ -168,20 +122,6 @@ impl LibAssembler for KclvmLibAssembler { KclvmLibAssembler::LLVM => LlvmLibAssembler::default().get_code_file_suffix(), } } - - #[inline] - fn lock_file_and_gen_lib( - &self, - compile_prog: &Program, - import_names: IndexMap>, - file: &Path, - ) -> String { - match &self { - KclvmLibAssembler::LLVM => { - LlvmLibAssembler::default().lock_file_and_gen_lib(compile_prog, import_names, file) - } - } - } } /// LlvmLibAssembler is mainly responsible for assembling the generated LLVM IR into a dynamic link library. @@ -220,7 +160,7 @@ impl LibAssembler for LlvmLibAssembler { lib_path: &str, ) -> String { // clean "*.ll" file path. - self.clean_path(&code_file_path.to_string()); + clean_path(&code_file_path.to_string()); // gen LLVM IR code into ".ll" file. emit_code( @@ -237,7 +177,7 @@ impl LibAssembler for LlvmLibAssembler { let mut cmd = Command::new(); let gen_lib_path = cmd.run_clang_single(code_file_path, lib_path); - self.clean_path(&code_file_path.to_string()); + clean_path(&code_file_path.to_string()); gen_lib_path } @@ -263,30 +203,29 @@ impl LibAssembler for LlvmLibAssembler { /// through KclvmLibAssembler for each thread. pub(crate) struct KclvmAssembler { thread_count: usize, + program: ast::Program, + scope: ProgramScope, + entry_file: String, + single_file_assembler: KclvmLibAssembler, } impl KclvmAssembler { - /// get the number of threads used in parallel multi-file compilation. - pub(crate) fn get_thread_count(self) -> usize { - return self.thread_count; - } - /// Constructs an KclvmAssembler instance with a default value 4 /// for the number of threads in multi-file compilation. #[inline] - pub(crate) fn new() -> Self { - Self { thread_count: 4 } - } - - /// Constructs an KclvmAssembler instance with a value - /// for the number of threads in multi-file compilation. - /// The number of threads must be greater than to 0. - #[inline] - pub(crate) fn new_with_thread_count(thread_count: usize) -> Self { - if thread_count <= 0 { - bug!("Illegal thread count in multi-file compilation"); + pub(crate) fn new( + program: ast::Program, + scope: ProgramScope, + entry_file: String, + single_file_assembler: KclvmLibAssembler, + ) -> Self { + Self { + thread_count: 4, + program, + scope, + entry_file, + single_file_assembler, } - Self { thread_count } } /// Clean up the path of the dynamic link libraries generated. @@ -338,18 +277,12 @@ impl KclvmAssembler { /// /// `gen_libs` will create multiple threads and call the method provided by [KclvmLibAssembler] in each thread /// to generate the dynamic link library in parallel. - pub(crate) fn gen_libs( - &self, - program: ast::Program, - scope: ProgramScope, - entry_file: &String, - single_file_assembler: KclvmLibAssembler, - ) -> Vec { + pub(crate) fn gen_libs(self) -> Vec { self.clean_path_for_genlibs( DEFAULT_IR_FILE, - &single_file_assembler.get_code_file_suffix(), + &self.single_file_assembler.get_code_file_suffix(), ); - let cache_dir = self.load_cache_dir(&program.root); + let cache_dir = self.load_cache_dir(&self.program.root); let mut compile_progs: IndexMap< String, ( @@ -358,19 +291,23 @@ impl KclvmAssembler { PathBuf, ), > = IndexMap::default(); - for (pkgpath, modules) in program.pkgs { + for (pkgpath, modules) in self.program.pkgs { let mut pkgs = HashMap::new(); pkgs.insert(pkgpath.clone(), modules); let compile_prog = ast::Program { - root: program.root.clone(), - main: program.main.clone(), + root: self.program.root.clone(), + main: self.program.main.clone(), pkgs, cmd_args: vec![], cmd_overrides: vec![], }; compile_progs.insert( pkgpath, - (compile_prog, scope.import_names.clone(), cache_dir.clone()), + ( + compile_prog, + self.scope.import_names.clone(), + cache_dir.clone(), + ), ); } let pool = ThreadPool::new(self.thread_count); @@ -378,10 +315,20 @@ impl KclvmAssembler { let prog_count = compile_progs.len(); for (pkgpath, (compile_prog, import_names, cache_dir)) in compile_progs { let tx = tx.clone(); - let temp_entry_file = entry_file.clone(); // clone a single file assembler for one thread. - let assembler = single_file_assembler.clone(); + let assembler = self.single_file_assembler.clone(); + + let code_file = self.entry_file.clone(); + let code_file_path = assembler.add_code_file_suffix(&code_file); + let lock_file_path = format!("{}.lock", code_file_path); + let lib_path = format!("{}{}", code_file, Command::get_lib_suffix()); + pool.execute(move || { + // Locking file for parallel code generation. + let mut file_lock = fslock::LockFile::open(&lock_file_path) + .expect(&format!("{} not found", lock_file_path)); + file_lock.lock().unwrap(); + let root = &compile_prog.root; let is_main_pkg = pkgpath == kclvm_ast::MAIN_PKG; // The main package does not perform cache reading and writing, @@ -391,9 +338,14 @@ impl KclvmAssembler { // be shared, so the cache of the main package is not read and // written. let lib_path = if is_main_pkg { - let file = PathBuf::from(&temp_entry_file); // generate dynamic link library for single file kcl program - assembler.lock_file_and_gen_lib(&compile_prog, import_names, &file) + assembler.assemble_lib( + &compile_prog, + import_names, + &code_file, + &code_file_path, + &lib_path, + ) } else { let file = cache_dir.join(&pkgpath); // Read the lib path cache @@ -417,9 +369,17 @@ impl KclvmAssembler { match lib_abs_path { Some(path) => path, None => { + let code_file = file.to_str().unwrap(); + let code_file_path = assembler.add_code_file_suffix(&code_file); + let lib_path = format!("{}{}", code_file, Command::get_lib_suffix()); // generate dynamic link library for single file kcl program - let lib_path = - assembler.lock_file_and_gen_lib(&compile_prog, import_names, &file); + let lib_path = assembler.assemble_lib( + &compile_prog, + import_names, + &code_file, + &code_file_path, + &lib_path, + ); let lib_relative_path = lib_path.replacen(root, ".", 1); save_pkg_cache( root, @@ -431,6 +391,7 @@ impl KclvmAssembler { } } }; + file_lock.unlock().unwrap(); tx.send(lib_path) .expect("channel will be there waiting for the pool"); }); @@ -447,14 +408,14 @@ impl KclvmAssembler { .unwrap(); lib_paths.push(lib_path); } - single_file_assembler.clean_lock_file(entry_file); + self.single_file_assembler.clean_lock_file(&self.entry_file); lib_paths } } -impl Default for KclvmAssembler { - #[inline] - fn default() -> Self { - Self::new() +#[inline] +pub(crate) fn clean_path(path: &str) { + if Path::new(path).exists() { + std::fs::remove_file(&path).unwrap(); } } diff --git a/kclvm/runner/src/lib.rs b/kclvm/runner/src/lib.rs index 30fbb2cb0..fa9a3f797 100644 --- a/kclvm/runner/src/lib.rs +++ b/kclvm/runner/src/lib.rs @@ -77,12 +77,13 @@ pub fn execute( let temp_entry_file = temp_file(temp_dir_path); // Generate libs - let lib_paths = assembler::KclvmAssembler::default().gen_libs( + let lib_paths = assembler::KclvmAssembler::new( program, scope, - &temp_entry_file, + temp_entry_file.clone(), KclvmLibAssembler::LLVM, - ); + ) + .gen_libs(); // Link libs let lib_suffix = Command::get_lib_suffix(); diff --git a/kclvm/runner/src/tests.rs b/kclvm/runner/src/tests.rs index 8b2f6687c..a5d8735fc 100644 --- a/kclvm/runner/src/tests.rs +++ b/kclvm/runner/src/tests.rs @@ -1,9 +1,12 @@ +use crate::assembler::clean_path; use crate::assembler::KclvmAssembler; use crate::assembler::KclvmLibAssembler; use crate::assembler::LibAssembler; use crate::temp_file; use crate::Command; use crate::{execute, runner::ExecProgramArgs}; +use anyhow::Context; +use anyhow::Result; use kclvm_ast::ast::{Module, Program}; use kclvm_config::settings::load_file; use kclvm_parser::load_program; @@ -12,6 +15,7 @@ use std::fs::create_dir_all; use std::panic::catch_unwind; use std::panic::set_hook; use std::path::{Path, PathBuf}; +use std::thread; use std::{ collections::HashMap, fs::{self, File}, @@ -19,7 +23,7 @@ use std::{ use tempfile::tempdir; use walkdir::WalkDir; -const EXEC_DATA_PATH: &str = "./src/exec_data/"; +const EXEC_DATA_PATH: &str = "src/exec_data/"; const TEST_CASES: &[&'static str; 5] = &[ "init_check_order_0", "init_check_order_1", @@ -43,19 +47,30 @@ const SETTINGS_FILE_TEST_CASE: &[&'static (&str, &str); 1] = &[&("settings_file/settings.yaml", "settings_file/settings.json")]; const EXPECTED_JSON_FILE_NAME: &str = "stdout.golden.json"; -const TEST_CASE_PATH: &str = "./src/test_datas"; +const TEST_CASE_PATH: &str = "src/test_datas"; const KCL_FILE_NAME: &str = "main.k"; const MAIN_PKG_NAME: &str = "__main__"; +const CARGO_PATH: &str = env!("CARGO_MANIFEST_DIR"); + +fn gen_full_path(rel_path: String) -> Result { + let mut cargo_file_path = PathBuf::from(CARGO_PATH); + cargo_file_path.push(&rel_path); + let full_path = cargo_file_path + .to_str() + .with_context(|| format!("No such file or directory '{}'", rel_path))?; + Ok(full_path.to_string()) +} /// Load test kcl file to ast.Program fn load_test_program(filename: String) -> Program { - let module = load_module(filename); + let module = kclvm_parser::parse_file(&filename, None).unwrap(); construct_program(module) } -/// Load test kcl file to ast.Module -fn load_module(filename: String) -> Module { - kclvm_parser::parse_file(&filename, None).unwrap() +fn parse_program(test_kcl_case_path: &str) -> Program { + let args = ExecProgramArgs::default(); + let opts = args.get_load_program_options(); + load_program(&[&test_kcl_case_path], Some(opts)).unwrap() } /// Construct ast.Program by ast.Module and default configuration. @@ -88,7 +103,11 @@ fn construct_pkg_lib_path( let mut result = vec![]; for (pkgpath, _) in &prog.pkgs { if pkgpath == "__main__" { - result.push(fs::canonicalize(format!("{}{}", main_path.to_string(), suffix)).unwrap()); + result.push(PathBuf::from(format!( + "{}{}", + main_path.to_string(), + suffix + ))); } else { result.push(cache_dir.join(format!("{}{}", pkgpath.clone(), suffix))); } @@ -118,30 +137,31 @@ fn execute_for_test(kcl_path: &String) -> String { execute(program, plugin_agent, &args).unwrap() } -fn gen_libs_for_test(entry_file: &str, test_kcl_case_path: &str) { - let args = ExecProgramArgs::default(); - let opts = args.get_load_program_options(); - - let mut prog = load_program(&[&test_kcl_case_path], Some(opts)).unwrap(); +fn gen_assembler(entry_file: &str, test_kcl_case_path: &str) -> KclvmAssembler { + let mut prog = parse_program(test_kcl_case_path); let scope = resolve_program(&mut prog); - - let assembler = KclvmAssembler::default(); - let prog_for_cache = prog.clone(); - - let lib_paths = assembler.gen_libs( - prog, + KclvmAssembler::new( + prog.clone(), scope, - &(entry_file.to_string()), + entry_file.to_string(), KclvmLibAssembler::LLVM, - ); + ) +} + +fn gen_libs_for_test(entry_file: &str, test_kcl_case_path: &str) { + let assembler = gen_assembler(entry_file, test_kcl_case_path); let expected_pkg_paths = construct_pkg_lib_path( - &prog_for_cache, + &parse_program(test_kcl_case_path), &assembler, PathBuf::from(entry_file).to_str().unwrap(), Command::get_lib_suffix(), ); + + let lib_paths = assembler.gen_libs(); + assert_eq!(lib_paths.len(), expected_pkg_paths.len()); + for pkg_path in &expected_pkg_paths { assert_eq!(pkg_path.exists(), true); } @@ -154,7 +174,7 @@ fn gen_libs_for_test(entry_file: &str, test_kcl_case_path: &str) { .unwrap(); assert_eq!(tmp_main_lib_path.exists(), true); - KclvmLibAssembler::LLVM.clean_path(&tmp_main_lib_path.to_str().unwrap()); + clean_path(&tmp_main_lib_path.to_str().unwrap()); assert_eq!(tmp_main_lib_path.exists(), false); } @@ -172,6 +192,7 @@ fn assemble_lib_for_test( // parse and resolve kcl let mut program = load_program(&files, Some(opts)).unwrap(); + let scope = resolve_program(&mut program); // tmp file @@ -239,7 +260,7 @@ fn test_assemble_lib_llvm() { let lib_path = std::path::Path::new(&lib_file); assert_eq!(lib_path.exists(), true); - assembler.clean_path(&lib_file); + clean_path(&lib_file); assert_eq!(lib_path.exists(), false); } } @@ -251,38 +272,37 @@ fn test_gen_libs() { let temp_dir_path = temp_dir.path().to_str().unwrap(); let temp_entry_file = temp_file(temp_dir_path); - let kcl_path = &format!("{}/{}/{}", TEST_CASE_PATH, case, KCL_FILE_NAME); - gen_libs_for_test(&format!("{}{}", temp_entry_file, "4gen_libs"), kcl_path); + let kcl_path = + gen_full_path(format!("{}/{}/{}", TEST_CASE_PATH, case, KCL_FILE_NAME)).unwrap(); + gen_libs_for_test(&format!("{}{}", temp_entry_file, "4gen_libs"), &kcl_path); } } #[test] -fn test_new_assembler_with_thread_count() { - let assembler = KclvmAssembler::new_with_thread_count(5); - assert_eq!(assembler.get_thread_count(), 5); -} - -#[test] -fn test_new_assembler_with_thread_count_invalid() { - set_hook(Box::new(|_| {})); - let result_new = catch_unwind(|| { - let _assembler_err = KclvmAssembler::new_with_thread_count(0); - }); - let err_msg = "Internal error, please report a bug to us. The error message is: Illegal thread count in multi-file compilation"; - match result_new { - Err(panic_err) => { - if let Some(s) = panic_err.downcast_ref::() { - assert_eq!(s, err_msg); - } +fn test_gen_libs_parallel() { + let gen_lib_1 = thread::spawn(|| { + for _ in 0..9 { + test_gen_libs(); } - _ => { - unreachable!() + }); + + let gen_lib_2 = thread::spawn(|| { + for _ in 0..9 { + test_gen_libs(); } - } + }); + + gen_lib_1.join().unwrap(); + gen_lib_2.join().unwrap(); } #[test] fn test_clean_path_for_genlibs() { + let mut prog = + parse_program("./src/test_datas/multi_file_compilation/import_abs_path/app-main/main.k"); + let scope = resolve_program(&mut prog); + let assembler = KclvmAssembler::new(prog, scope, String::new(), KclvmLibAssembler::LLVM); + let temp_dir = tempdir().unwrap(); let temp_dir_path = temp_dir.path().to_str().unwrap(); let tmp_file_path = &temp_file(temp_dir_path); @@ -296,7 +316,7 @@ fn test_clean_path_for_genlibs() { let path = std::path::Path::new(file_name); assert_eq!(path.exists(), true); - KclvmAssembler::new().clean_path_for_genlibs(file_name, file_suffix); + assembler.clean_path_for_genlibs(file_name, file_suffix); assert_eq!(path.exists(), false); let test1 = &format!("{}{}", file_name, ".test1.ll"); @@ -309,7 +329,7 @@ fn test_clean_path_for_genlibs() { assert_eq!(path1.exists(), true); assert_eq!(path2.exists(), true); - KclvmAssembler::new().clean_path_for_genlibs(file_name, file_suffix); + assembler.clean_path_for_genlibs(file_name, file_suffix); assert_eq!(path1.exists(), false); assert_eq!(path2.exists(), false); } diff --git a/kclvm/sema/src/ty/into.rs b/kclvm/sema/src/ty/into.rs index ac22c021b..d46fa153b 100644 --- a/kclvm/sema/src/ty/into.rs +++ b/kclvm/sema/src/ty/into.rs @@ -77,7 +77,7 @@ impl Type { } float_str } - TypeKind::StrLit(v) => (format!("\"{}\"", v.replace('"', "\\\""))), + TypeKind::StrLit(v) => format!("\"{}\"", v.replace('"', "\\\"")), TypeKind::List(item_ty) => format!("[{}]", item_ty.into_type_annotation_str()), TypeKind::Dict(key_ty, val_ty) => { format!(