Skip to content

Commit

Permalink
Disable DCA and writing diagnostics on did_change events (#5555)
Browse files Browse the repository at this point in the history
## Description
This PR does 3 optimizations. The timings below are measured against the
LSP benchmarking project.

1. Disable running DCA & control flow analysis, for did_change events
`39.544ms`
2. Disable running collect_types_metadata for did_change events
`3.522ms`
3. Only write the diagnostics res to self.diagnostics.write() on
did_open & did_save `21.135ms`

I also had to increase the frequency that we are calling GC to every 3rd
keystroke as during stress tests I was occasionally getting stack
overflows otherwise.

related to #5445

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
  • Loading branch information
JoshuaBatty authored and sdankel committed Feb 8, 2024
1 parent ff33454 commit 44e790e
Show file tree
Hide file tree
Showing 15 changed files with 168 additions and 80 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions forc-pkg/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,6 @@ pub struct BuildProfile {
pub optimization_level: OptLevel,
#[serde(default)]
pub experimental: ExperimentalFlags,
pub lsp_mode: bool,
}

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Default)]
Expand Down Expand Up @@ -761,7 +760,6 @@ impl BuildProfile {
experimental: ExperimentalFlags {
new_encoding: false,
},
lsp_mode: false,
}
}

Expand All @@ -784,7 +782,6 @@ impl BuildProfile {
experimental: ExperimentalFlags {
new_encoding: false,
},
lsp_mode: false,
}
}
}
Expand Down
9 changes: 4 additions & 5 deletions forc-pkg/src/pkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ use sway_core::{
semantic_analysis::namespace,
source_map::SourceMap,
transform::AttributeKind,
BuildTarget, Engines, FinalizedEntry,
BuildTarget, Engines, FinalizedEntry, LspConfig,
};
use sway_error::{error::CompileError, handler::Handler, warning::CompileWarning};
use sway_types::constants::{CORE, PRELUDE, STD};
Expand Down Expand Up @@ -1567,8 +1567,7 @@ pub fn sway_build_config(
.with_optimization_level(build_profile.optimization_level)
.with_experimental(sway_core::ExperimentalFlags {
new_encoding: build_profile.experimental.new_encoding,
})
.with_lsp_mode(build_profile.lsp_mode);
});
Ok(build_config)
}

Expand Down Expand Up @@ -2598,7 +2597,7 @@ pub fn check(
plan: &BuildPlan,
build_target: BuildTarget,
terse_mode: bool,
lsp_mode: bool,
lsp_mode: Option<LspConfig>,
include_tests: bool,
engines: &Engines,
retrigger_compilation: Option<Arc<AtomicBool>>,
Expand Down Expand Up @@ -2647,7 +2646,7 @@ pub fn check(
&profile,
)?
.with_include_tests(include_tests)
.with_lsp_mode(lsp_mode);
.with_lsp_mode(lsp_mode.clone());

let input = manifest.entry_string()?;
let handler = Handler::default();
Expand Down
2 changes: 1 addition & 1 deletion forc-plugins/forc-doc/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ pub fn compile_html(
&plan,
BuildTarget::default(),
build_instructions.silent,
None,
tests_enabled,
false,
&engines,
None,
)?;
Expand Down
2 changes: 1 addition & 1 deletion forc/src/ops/forc_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub fn check(command: CheckCommand, engines: &Engines) -> Result<(Option<ty::TyP
&plan,
build_target,
terse_mode,
false,
None,
tests_enabled,
engines,
None,
Expand Down
15 changes: 12 additions & 3 deletions sway-core/src/build_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub struct BuildConfig {
pub time_phases: bool,
pub metrics_outfile: Option<String>,
pub experimental: ExperimentalFlags,
pub lsp_mode: bool,
pub lsp_mode: Option<LspConfig>,
}

impl BuildConfig {
Expand Down Expand Up @@ -103,7 +103,7 @@ impl BuildConfig {
metrics_outfile: None,
optimization_level: OptLevel::Opt0,
experimental: ExperimentalFlags::default(),
lsp_mode: false,
lsp_mode: None,
}
}

Expand Down Expand Up @@ -182,7 +182,7 @@ impl BuildConfig {
}
}

pub fn with_lsp_mode(self, lsp_mode: bool) -> Self {
pub fn with_lsp_mode(self, lsp_mode: Option<LspConfig>) -> Self {
Self { lsp_mode, ..self }
}

Expand All @@ -196,6 +196,15 @@ pub struct ExperimentalFlags {
pub new_encoding: bool,
}

#[derive(Clone, Debug, Default)]
pub struct LspConfig {
// This is set to true if compilation was triggered by a didChange LSP event. In this case, we
// bypass collecting type metadata and skip DCA.
//
// This is set to false if compilation was triggered by a didSave or didOpen LSP event.
pub optimized_build: bool,
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
107 changes: 60 additions & 47 deletions sway-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use crate::source_map::SourceMap;
pub use asm_generation::from_ir::compile_ir_to_asm;
use asm_generation::FinalizedAsm;
pub use asm_generation::{CompiledBytecode, FinalizedEntry};
pub use build_config::{BuildConfig, BuildTarget, OptLevel};
pub use build_config::{BuildConfig, BuildTarget, LspConfig, OptLevel};
use control_flow_analysis::ControlFlowGraph;
use metadata::MetadataManager;
use query_engine::{ModuleCacheKey, ModulePath, ProgramsCacheEntry};
Expand Down Expand Up @@ -480,6 +480,7 @@ pub fn parsed_to_ast(
retrigger_compilation: Option<Arc<AtomicBool>>,
) -> Result<ty::TyProgram, ErrorEmitted> {
let experimental = build_config.map(|x| x.experimental).unwrap_or_default();
let lsp_config = build_config.map(|x| x.lsp_mode.clone()).unwrap_or_default();

// Type check the program.
let typed_program_opt = ty::TyProgram::type_check(
Expand All @@ -491,16 +492,15 @@ pub fn parsed_to_ast(
build_config,
);

check_should_abort(handler, retrigger_compilation.clone())?;

// Only clear the parsed AST nodes if we are running a regular compilation pipeline.
// LSP needs these to build its token map, and they are cleared by `clear_module` as
// part of the LSP garbage collection functionality instead.
let lsp_mode = build_config.map(|x| x.lsp_mode).unwrap_or_default();
if !lsp_mode {
if lsp_config.is_none() {
engines.pe().clear();
}

check_should_abort(handler, retrigger_compilation.clone())?;

let mut typed_program = match typed_program_opt {
Ok(typed_program) => typed_program,
Err(e) => return Err(e),
Expand All @@ -516,51 +516,62 @@ pub fn parsed_to_ast(
}
};

// Collect information about the types used in this program
let types_metadata_result = typed_program.collect_types_metadata(
handler,
&mut CollectTypesMetadataContext::new(engines, experimental),
);
let types_metadata = match types_metadata_result {
Ok(types_metadata) => types_metadata,
Err(e) => {
handler.dedup();
return Err(e);
}
};
// Skip collecting metadata if we triggered an optimised build from LSP.
let types_metadata = if !lsp_config
.as_ref()
.map(|lsp| lsp.optimized_build)
.unwrap_or(false)
{
// Collect information about the types used in this program
let types_metadata_result = typed_program.collect_types_metadata(
handler,
&mut CollectTypesMetadataContext::new(engines, experimental),
);
let types_metadata = match types_metadata_result {
Ok(types_metadata) => types_metadata,
Err(e) => {
handler.dedup();
return Err(e);
}
};

typed_program
.logged_types
.extend(types_metadata.iter().filter_map(|m| match m {
TypeMetadata::LoggedType(log_id, type_id) => Some((*log_id, *type_id)),
_ => None,
}));

typed_program
.messages_types
.extend(types_metadata.iter().filter_map(|m| match m {
TypeMetadata::MessageType(message_id, type_id) => Some((*message_id, *type_id)),
_ => None,
}));

let (print_graph, print_graph_url_format) = match build_config {
Some(cfg) => (
cfg.print_dca_graph.clone(),
cfg.print_dca_graph_url_format.clone(),
),
None => (None, None),
};
typed_program
.logged_types
.extend(types_metadata.iter().filter_map(|m| match m {
TypeMetadata::LoggedType(log_id, type_id) => Some((*log_id, *type_id)),
_ => None,
}));

check_should_abort(handler, retrigger_compilation.clone())?;
typed_program
.messages_types
.extend(types_metadata.iter().filter_map(|m| match m {
TypeMetadata::MessageType(message_id, type_id) => Some((*message_id, *type_id)),
_ => None,
}));

let (print_graph, print_graph_url_format) = match build_config {
Some(cfg) => (
cfg.print_dca_graph.clone(),
cfg.print_dca_graph_url_format.clone(),
),
None => (None, None),
};

// Perform control flow analysis and extend with any errors.
let _ = perform_control_flow_analysis(
handler,
engines,
&typed_program,
print_graph,
print_graph_url_format,
);
check_should_abort(handler, retrigger_compilation.clone())?;

// Perform control flow analysis and extend with any errors.
let _ = perform_control_flow_analysis(
handler,
engines,
&typed_program,
print_graph,
print_graph_url_format,
);

types_metadata
} else {
vec![]
};

// Evaluate const declarations, to allow storage slots initialization with consts.
let mut ctx = Context::new(
Expand Down Expand Up @@ -713,6 +724,8 @@ pub fn compile_to_ast(
query_engine.insert_programs_cache_entry(cache_entry);
}

check_should_abort(handler, retrigger_compilation.clone())?;

Ok(programs)
}

Expand Down
1 change: 1 addition & 0 deletions sway-lsp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ futures = { version = "0.3", default-features = false, features = [
"async-await",
] }
pretty_assertions = "1.4.0"
rand = "0.8"
regex = "^1.10.2"
sway-lsp-test-utils = { path = "tests/utils" }
tikv-jemallocator = "0.5"
Expand Down
11 changes: 8 additions & 3 deletions sway-lsp/benches/lsp_benchmarks/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,33 @@ const NUM_DID_CHANGE_ITERATIONS: usize = 10;
fn benchmarks(c: &mut Criterion) {
// Load the test project
let uri = Url::from_file_path(super::benchmark_dir().join("src/main.sw")).unwrap();
let mut lsp_mode = Some(sway_core::LspConfig {
optimized_build: false,
});
c.bench_function("compile", |b| {
b.iter(|| {
let engines = Engines::default();
let _ = black_box(session::compile(&uri, &engines, None).unwrap());
let _ = black_box(session::compile(&uri, &engines, None, lsp_mode.clone()).unwrap());
})
});

c.bench_function("traverse", |b| {
let engines = Engines::default();
let results = black_box(session::compile(&uri, &engines, None).unwrap());
let results = black_box(session::compile(&uri, &engines, None, lsp_mode.clone()).unwrap());
let session = Arc::new(session::Session::new());
b.iter(|| {
let _ =
black_box(session::traverse(results.clone(), &engines, session.clone()).unwrap());
})
});

lsp_mode.as_mut().unwrap().optimized_build = true;
c.bench_function("did_change_with_caching", |b| {
let engines = Engines::default();
b.iter(|| {
for _ in 0..NUM_DID_CHANGE_ITERATIONS {
let _ = black_box(session::compile(&uri, &engines, None).unwrap());
let _ =
black_box(session::compile(&uri, &engines, None, lsp_mode.clone()).unwrap());
}
})
});
Expand Down
12 changes: 11 additions & 1 deletion sway-lsp/benches/lsp_benchmarks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,21 @@ use sway_lsp::core::session::{self, Session};

pub async fn compile_test_project() -> (Url, Arc<Session>) {
let session = Arc::new(Session::new());
let lsp_mode = Some(sway_core::LspConfig {
optimized_build: false,
});
// Load the test project
let uri = Url::from_file_path(benchmark_dir().join("src/main.sw")).unwrap();
session.handle_open_file(&uri).await;
// Compile the project
session::parse_project(&uri, &session.engines.read(), None, session.clone()).unwrap();
session::parse_project(
&uri,
&session.engines.read(),
None,
lsp_mode,
session.clone(),
)
.unwrap();
(uri, session)
}

Expand Down
Loading

0 comments on commit 44e790e

Please sign in to comment.