Skip to content

Commit

Permalink
feat: Extract parsing to its own pass and do it in parallel (#4063)
Browse files Browse the repository at this point in the history
# Description
Rework of #3849 after
file_manager_with_stdlib was created. Also as @kevaundray suggested
extracted the parsing above the context.

Resolves #3838
## Problem\*
Parsing is currently done when collecting, allowing only to parse files
used, but making it very difficult to do parallel parsing or to have
cached parsing.

## Summary\*

This PR extracts parsing to its own pass, and makes the Context take the
parsed files. The creator of the context is the one in charge to do the
parsing of the file manager, so Nargo, the LSP and wasm need to handle
the parsed files. This PR uses rayon to do parallel parsing in Nargo &
LSP. It reduces the time taken to process the on save notification in
the LSP from ~700ms on protocol circuits to ~250ms.

With parsing being in its own pass, this opens the door for the LSP to
cache parsing. It has access to the file manager and the parsed files,
so it can detect which files have changed on disk and only parse those
when necessary.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Koby Hall <102518238+kobyhallx@users.noreply.github.com>
  • Loading branch information
sirasistant and kobyhallx authored Jan 18, 2024
1 parent 5999d0e commit 569cbbc
Show file tree
Hide file tree
Showing 28 changed files with 208 additions and 69 deletions.
4 changes: 4 additions & 0 deletions compiler/fm/src/file_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ impl FileMap {
pub fn get_file_id(&self, file_name: &PathString) -> Option<FileId> {
self.name_to_id.get(file_name).cloned()
}

pub fn all_file_ids(&self) -> impl Iterator<Item = &FileId> {
self.name_to_id.values()
}
}
impl Default for FileMap {
fn default() -> Self {
Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_driver/tests/stdlib_warnings.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::path::Path;

use noirc_driver::{file_manager_with_stdlib, prepare_crate, ErrorsAndWarnings};
use noirc_frontend::hir::Context;
use noirc_frontend::hir::{def_map::parse_file, Context};

#[test]
fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings> {
Expand All @@ -15,8 +15,13 @@ fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings>
file_manager.add_file_with_source(file_name, source.to_owned()).expect(
"Adding source buffer to file manager should never fail when file manager is empty",
);
let parsed_files = file_manager
.as_file_map()
.all_file_ids()
.map(|&file_id| (file_id, parse_file(&file_manager, file_id)))
.collect();

let mut context = Context::new(file_manager);
let mut context = Context::new(file_manager, parsed_files);
let root_crate_id = prepare_crate(&mut context, file_name);

let ((), warnings) = noirc_driver::check_crate(&mut context, root_crate_id, false, false)?;
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use super::{
},
errors::{DefCollectorErrorKind, DuplicateType},
};
use crate::hir::def_map::{parse_file, LocalModuleId, ModuleData, ModuleId};
use crate::hir::def_map::{LocalModuleId, ModuleData, ModuleId};
use crate::hir::resolution::import::ImportDirective;
use crate::hir::Context;

Expand Down Expand Up @@ -555,7 +555,7 @@ impl<'a> ModCollector<'a> {
context.visited_files.insert(child_file_id, location);

// Parse the AST for the module we just found and then recursively look for it's defs
let (ast, parsing_errors) = parse_file(&context.file_manager, child_file_id);
let (ast, parsing_errors) = context.parsed_file_results(child_file_id);
let ast = ast.into_sorted();

errors.extend(
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl CrateDefMap {

// First parse the root file.
let root_file_id = context.crate_graph[crate_id].root_file_id;
let (ast, parsing_errors) = parse_file(&context.file_manager, root_file_id);
let (ast, parsing_errors) = context.parsed_file_results(root_file_id);
let mut ast = ast.into_sorted();

for macro_processor in &macro_processors {
Expand Down
28 changes: 23 additions & 5 deletions compiler/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,22 @@ pub mod type_check;
use crate::graph::{CrateGraph, CrateId};
use crate::hir_def::function::FuncMeta;
use crate::node_interner::{FuncId, NodeInterner, StructId};
use crate::parser::ParserError;
use crate::ParsedModule;
use def_map::{Contract, CrateDefMap};
use fm::FileManager;
use noirc_errors::Location;
use std::borrow::Cow;
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};

use self::def_map::TestFunction;

pub type ParsedFiles = HashMap<fm::FileId, (ParsedModule, Vec<ParserError>)>;

/// Helper object which groups together several useful context objects used
/// during name resolution. Once name resolution is finished, only the
/// def_interner is required for type inference and monomorphization.
pub struct Context<'file_manager> {
pub struct Context<'file_manager, 'parsed_files> {
pub def_interner: NodeInterner,
pub crate_graph: CrateGraph,
pub(crate) def_maps: BTreeMap<CrateId, CrateDefMap>,
Expand All @@ -30,6 +34,11 @@ pub struct Context<'file_manager> {
/// A map of each file that already has been visited from a prior `mod foo;` declaration.
/// This is used to issue an error if a second `mod foo;` is declared to the same file.
pub visited_files: BTreeMap<fm::FileId, Location>,

// A map of all parsed files.
// Same as the file manager, we take ownership of the parsed files in the WASM context.
// Parsed files is also read only.
pub parsed_files: Cow<'parsed_files, ParsedFiles>,
}

#[derive(Debug, Copy, Clone)]
Expand All @@ -39,27 +48,36 @@ pub enum FunctionNameMatch<'a> {
Contains(&'a str),
}

impl Context<'_> {
pub fn new(file_manager: FileManager) -> Context<'static> {
impl Context<'_, '_> {
pub fn new(file_manager: FileManager, parsed_files: ParsedFiles) -> Context<'static, 'static> {
Context {
def_interner: NodeInterner::default(),
def_maps: BTreeMap::new(),
visited_files: BTreeMap::new(),
crate_graph: CrateGraph::default(),
file_manager: Cow::Owned(file_manager),
parsed_files: Cow::Owned(parsed_files),
}
}

pub fn from_ref_file_manager(file_manager: &FileManager) -> Context<'_> {
pub fn from_ref_file_manager<'file_manager, 'parsed_files>(
file_manager: &'file_manager FileManager,
parsed_files: &'parsed_files ParsedFiles,
) -> Context<'file_manager, 'parsed_files> {
Context {
def_interner: NodeInterner::default(),
def_maps: BTreeMap::new(),
visited_files: BTreeMap::new(),
crate_graph: CrateGraph::default(),
file_manager: Cow::Borrowed(file_manager),
parsed_files: Cow::Borrowed(parsed_files),
}
}

pub fn parsed_file_results(&self, file_id: fm::FileId) -> (ParsedModule, Vec<ParserError>) {
self.parsed_files.get(&file_id).expect("noir file wasn't parsed").clone()
}

/// Returns the CrateDefMap for a given CrateId.
/// It is perfectly valid for the compiler to look
/// up a CrateDefMap and it is not available.
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ mod test {
) -> (ParsedModule, Context, Vec<(CompilationError, FileId)>) {
let root = std::path::Path::new("/");
let fm = FileManager::new(root);
let mut context = Context::new(fm);
let mut context = Context::new(fm, Default::default());
context.def_interner.populate_dummy_operator_traits();
let root_file_id = FileId::dummy();
let root_crate_id = context.crate_graph.add_crate_root(root_file_id);
Expand Down
19 changes: 13 additions & 6 deletions compiler/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use noirc_driver::{
};
use noirc_frontend::{
graph::{CrateId, CrateName},
hir::Context,
hir::{def_map::parse_file, Context, ParsedFiles},
};
use serde::Deserialize;
use std::{collections::HashMap, path::Path};
Expand Down Expand Up @@ -147,6 +147,10 @@ impl PathToFileSourceMap {
}
}

pub(crate) fn parse_all(fm: &FileManager) -> ParsedFiles {
fm.as_file_map().all_file_ids().map(|&file_id| (file_id, parse_file(fm, file_id))).collect()
}

pub enum CompileResult {
Contract { contract: ContractArtifact, debug: DebugArtifact },
Program { program: ProgramArtifact, debug: DebugArtifact },
Expand All @@ -169,8 +173,8 @@ pub fn compile(
};

let fm = file_manager_with_source_map(file_source_map);

let mut context = Context::new(fm);
let parsed_files = parse_all(&fm);
let mut context = Context::new(fm, parsed_files);

let path = Path::new(&entry_point);
let crate_id = prepare_crate(&mut context, path);
Expand Down Expand Up @@ -307,15 +311,18 @@ mod test {

use crate::compile::PathToFileSourceMap;

use super::{file_manager_with_source_map, process_dependency_graph, DependencyGraph};
use super::{
file_manager_with_source_map, parse_all, process_dependency_graph, DependencyGraph,
};
use std::{collections::HashMap, path::Path};

fn setup_test_context(source_map: PathToFileSourceMap) -> Context<'static> {
fn setup_test_context(source_map: PathToFileSourceMap) -> Context<'static, 'static> {
let mut fm = file_manager_with_source_map(source_map);
// Add this due to us calling prepare_crate on "/main.nr" below
fm.add_file_with_source(Path::new("/main.nr"), "fn foo() {}".to_string());
let parsed_files = parse_all(&fm);

let mut context = Context::new(fm);
let mut context = Context::new(fm, parsed_files);
prepare_crate(&mut context, Path::new("/main.nr"));

context
Expand Down
13 changes: 8 additions & 5 deletions compiler/wasm/src/compile_new.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::compile::{
file_manager_with_source_map, generate_contract_artifact, generate_program_artifact,
file_manager_with_source_map, generate_contract_artifact, generate_program_artifact, parse_all,
JsCompileResult, PathToFileSourceMap,
};
use crate::errors::{CompileError, JsCompileError};
Expand All @@ -20,7 +20,7 @@ use wasm_bindgen::prelude::wasm_bindgen;
pub struct CompilerContext {
// `wasm_bindgen` currently doesn't allow lifetime parameters on structs so we must use a `'static` lifetime.
// `Context` must then own the `FileManager` to satisfy this lifetime.
context: Context<'static>,
context: Context<'static, 'static>,
}

#[wasm_bindgen(js_name = "CrateId")]
Expand All @@ -34,7 +34,9 @@ impl CompilerContext {
console_error_panic_hook::set_once();

let fm = file_manager_with_source_map(source_map);
CompilerContext { context: Context::new(fm) }
let parsed_files = parse_all(&fm);

CompilerContext { context: Context::new(fm, parsed_files) }
}

#[cfg(test)]
Expand Down Expand Up @@ -231,7 +233,7 @@ mod test {
use noirc_driver::prepare_crate;
use noirc_frontend::hir::Context;

use crate::compile::{file_manager_with_source_map, PathToFileSourceMap};
use crate::compile::{file_manager_with_source_map, parse_all, PathToFileSourceMap};

use std::path::Path;

Expand All @@ -241,8 +243,9 @@ mod test {
let mut fm = file_manager_with_source_map(source_map);
// Add this due to us calling prepare_crate on "/main.nr" below
fm.add_file_with_source(Path::new("/main.nr"), "fn foo() {}".to_string());
let parsed_files = parse_all(&fm);

let mut context = Context::new(fm);
let mut context = Context::new(fm, parsed_files);
prepare_crate(&mut context, Path::new("/main.nr"));

CompilerContext { context }
Expand Down
7 changes: 4 additions & 3 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use async_lsp::{
};
use fm::codespan_files as files;
use lsp_types::CodeLens;
use nargo::workspace::Workspace;
use nargo::{parse_all, workspace::Workspace};
use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{file_manager_with_stdlib, prepare_crate, NOIR_ARTIFACT_VERSION_STRING};
use noirc_frontend::{
Expand Down Expand Up @@ -227,15 +227,16 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result<Work
/// Use case for this is the LSP server and code lenses
/// which operate on single file and need to understand this file
/// in order to offer code lenses to the user
fn prepare_source(source: String) -> (Context<'static>, CrateId) {
fn prepare_source(source: String) -> (Context<'static, 'static>, CrateId) {
let root = Path::new("");
let file_name = Path::new("main.nr");
let mut file_manager = file_manager_with_stdlib(root);
file_manager.add_file_with_source(file_name, source).expect(
"Adding source buffer to file manager should never fail when file manager is empty",
);
let parsed_files = parse_all(&file_manager);

let mut context = Context::new(file_manager);
let mut context = Context::new(file_manager, parsed_files);
let root_crate_id = prepare_crate(&mut context, file_name);

(context, root_crate_id)
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::ops::ControlFlow;

use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use nargo::{insert_all_files_for_workspace_into_file_manager, prepare_package};
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all, prepare_package};
use noirc_driver::{check_crate, file_manager_with_stdlib};
use noirc_errors::{DiagnosticKind, FileDiagnostic};

Expand Down Expand Up @@ -130,11 +130,13 @@ fn process_noir_document(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let diagnostics: Vec<_> = workspace
.into_iter()
.flat_map(|package| -> Vec<Diagnostic> {
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
prepare_package(&workspace_file_manager, &parsed_files, package);

let file_diagnostics = match check_crate(&mut context, crate_id, false, false) {
Ok(((), warnings)) => warnings,
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/requests/goto_declaration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use async_lsp::{ErrorCode, ResponseError};

use lsp_types::request::{GotoDeclarationParams, GotoDeclarationResponse};

use nargo::insert_all_files_for_workspace_into_file_manager;
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all};
use noirc_driver::file_manager_with_stdlib;

use super::{position_to_byte_index, to_lsp_location};
Expand Down Expand Up @@ -36,8 +36,10 @@ fn on_goto_definition_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
nargo::prepare_package(&workspace_file_manager, &parsed_files, package);

let interner;
if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) {
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/requests/goto_definition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use async_lsp::{ErrorCode, ResponseError};

use lsp_types::request::GotoTypeDefinitionParams;
use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse};
use nargo::insert_all_files_for_workspace_into_file_manager;
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all};
use noirc_driver::file_manager_with_stdlib;

use super::{position_to_byte_index, to_lsp_location};
Expand Down Expand Up @@ -44,8 +44,10 @@ fn on_goto_definition_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
nargo::prepare_package(&workspace_file_manager, &parsed_files, package);

let interner;
if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) {
Expand Down
6 changes: 5 additions & 1 deletion tooling/lsp/src/requests/profile_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ use std::{

use acvm::ExpressionWidth;
use async_lsp::{ErrorCode, ResponseError};
use nargo::{artifacts::debug::DebugArtifact, insert_all_files_for_workspace_into_file_manager};
use nargo::{
artifacts::debug::DebugArtifact, insert_all_files_for_workspace_into_file_manager, parse_all,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
file_manager_with_stdlib, CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING,
Expand Down Expand Up @@ -52,6 +54,7 @@ fn on_profile_run_request_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

// Since we filtered on crate name, this should be the only item in the iterator
match workspace.into_iter().next() {
Expand All @@ -66,6 +69,7 @@ fn on_profile_run_request_inner(

let (compiled_programs, compiled_contracts) = nargo::ops::compile_workspace(
&workspace_file_manager,
&parsed_files,
&workspace,
&binary_packages,
&contract_packages,
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use async_lsp::{ErrorCode, ResponseError};
use nargo::{
insert_all_files_for_workspace_into_file_manager,
ops::{run_test, TestStatus},
prepare_package,
parse_all, prepare_package,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
Expand Down Expand Up @@ -52,11 +52,13 @@ fn on_test_run_request_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

// Since we filtered on crate name, this should be the only item in the iterator
match workspace.into_iter().next() {
Some(package) => {
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
prepare_package(&workspace_file_manager, &parsed_files, package);
if check_crate(&mut context, crate_id, false, false).is_err() {
let result = NargoTestRunResult {
id: params.id.clone(),
Expand Down
Loading

0 comments on commit 569cbbc

Please sign in to comment.