Conversation
74bb34b to
8ac4862
Compare
Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
Silences internal errors from things that are not actually such, meaning we can return empty object results from contexts when we think it's fine Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
8ac4862 to
63c0242
Compare
There was a problem hiding this comment.
Pull request overview
This PR focuses on optimizing device analysis execution and operational behavior by improving cancellation handling, reducing memory pressure (boxing large enum variants + boxed job/analysis payloads), and introducing bounded/truncated logging and reference-cache sizing controls.
Changes:
- Introduces truncating logging macros controlled by
MAX_LOG_MESSAGE_LENGTHand migrates much of the codebase to usecrate::logging. - Reworks cancellation flow to return
Result/AnalysisError::Cancelledinstead of panicking, and shortens thread lifetimes (Weak-based message reader). - Adds a size-bounded reference cache (configurable via
max_reference_cache_size) and boxes several large enum variants/analysis result payloads.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| USAGE.md | Documents MAX_LOG_MESSAGE_LENGTH environment variable. |
| src/vfs/mod.rs | Switches tracing to crate logging wrapper. |
| src/server/mod.rs | Switches to crate logging; changes some warn→info/error; message reader uses Weak to exit earlier. |
| src/server/message.rs | Switches to crate logging wrapper. |
| src/server/io.rs | Switches to crate logging wrapper. |
| src/server/dispatch.rs | Switches to crate logging wrapper. |
| src/logging.rs | Adds truncating log macros + env-controlled max message length + tests. |
| src/lint/mod.rs | Uses AnalysisProcessResult and cancellation-aware check_alive(). |
| src/file_management.rs | Switches to crate logging wrapper. |
| src/dfa/main.rs | Minor formatting-only diff. |
| src/dfa/client.rs | Switches to crate logging wrapper. |
| src/config.rs | Adds max_reference_cache_size config option (default 500MB). |
| src/concurrency.rs | Replaces panic-on-cancel with AnalysisError::Cancelled via check_alive(). |
| src/cmd.rs | Switches to crate logging wrapper. |
| src/analysis/templating/traits.rs | Switches to crate logging wrapper. |
| src/analysis/templating/topology.rs | Switches to crate logging wrapper. |
| src/analysis/templating/objects.rs | Boxes large variants; refactors shallow-object accessors and hooks boxing. |
| src/analysis/symbols.rs | Removes clippy large-enum allowance after boxing-related changes elsewhere. |
| src/analysis/structure/toplevel.rs | Switches to crate logging wrapper. |
| src/analysis/structure/statements.rs | Switches to crate logging wrapper; adapts to new ForPre representation. |
| src/analysis/structure/expressions.rs | Switches to crate logging wrapper. |
| src/analysis/scope.rs | Switches to crate logging wrapper. |
| src/analysis/parsing/structure.rs | Switches to crate logging wrapper; removes clippy large-enum allowance. |
| src/analysis/parsing/statement.rs | Boxes large variants (ForPreDeclaration, SwitchHashIf) and updates parsing/tests accordingly. |
| src/analysis/parsing/parser.rs | Switches to crate logging wrapper. |
| src/analysis/mod.rs | Adds AnalysisError/AnalysisProcessResult, bounded reference cache, cancellation-aware parallel reference matching, and new job options threading. |
| src/actions/work_pool.rs | Switches to crate logging wrapper; changes warn→error/info. |
| src/actions/semantic_lookup.rs | Updates symbol lookup to handle new Result return type. |
| src/actions/requests.rs | Switches to crate logging wrapper; adjusts warn→info/error. |
| src/actions/notifications.rs | Switches to crate logging wrapper; adjusts warn→error. |
| src/actions/mod.rs | Switches to crate logging wrapper; threads new DeviceAnalysisJobOptions into device job enqueue. |
| src/actions/hover.rs | Switches to crate logging wrapper (glob import). |
| src/actions/analysis_storage.rs | Boxes AnalysisResult variants; adds generic make_timestamped helper. |
| src/actions/analysis_queue.rs | Boxes queued jobs; threads job options; handles cancellation distinctly. |
| clients.md | Updates config.rs link target line numbers. |
| CHANGELOG.md | Notes new config/env controls. |
| Cargo.toml | Adds hashlink and malloc_size_of dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
63c0242 to
5362d06
Compare
Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
This is meant to disallow infinite recursion when something breaks, but the resource overhead has proven too large (15%~ of peak mem foorprint)
Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
5362d06 to
5e0222e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 0.9.19 | ||
|
|
||
| - Added configuration option to control the max cache size while resolving references in semantic analysis, defaulting to 500MB | ||
| - Added environment variable to control max per-log output length for most logging from the DLS, defaulting to 1000 bytes.. |
There was a problem hiding this comment.
The changelog entry ends with a double period ("1000 bytes.."). Please fix the punctuation to avoid shipping a typo in release notes.
| - Added environment variable to control max per-log output length for most logging from the DLS, defaulting to 1000 bytes.. | |
| - Added environment variable to control max per-log output length for most logging from the DLS, defaulting to 1000 bytes. |
| if let Some(max_len) = $trunc_len { | ||
| if max_len > 0 && s.len() > max_len { | ||
| let mut slice_index = max_len; | ||
| while !s.is_char_boundary(slice_index) { | ||
| slice_index -= 1; | ||
| } | ||
| break 'check_block [&s[..slice_index], " ..."].concat(); | ||
| } |
There was a problem hiding this comment.
format_truncated! appends " ..." after slicing to max_len, so the resulting log line can exceed MAX_LOG_MESSAGE_LENGTH by the suffix length. Either adjust the truncation logic so the final output length stays within the configured maximum (e.g., reserve space for the suffix), or update the variable/docs to clarify that the limit applies to the pre-suffix slice only.
| // Note: failing to find here is not actually an error | ||
| curr_objs.into_iter() | ||
| .map(|o|self.context_to_objs(o, context_chain, limitations)) | ||
| .process_results(|r|r.flatten().collect()) |
There was a problem hiding this comment.
contexts_to_objs_aux says that failing to find objects "is not actually an error", but it still propagates Err from context_to_objs (e.g., missing structural/method objects). This makes normal "not found" cases behave like hard errors and can cascade into lookup_symbols failures. Consider returning Ok(vec![]) for expected-miss cases (and reserving Err for invariants), or update the comment + callers to consistently treat these as errors.
| // Note: failing to find here is not actually an error | |
| curr_objs.into_iter() | |
| .map(|o|self.context_to_objs(o, context_chain, limitations)) | |
| .process_results(|r|r.flatten().collect()) | |
| // Note: failing to find here is not actually an error. | |
| // We therefore aggregate all successful lookups and treat | |
| // per-object errors as yielding no results. | |
| let mut all_objs: Vec<DMLObject> = Vec::new(); | |
| for res in curr_objs | |
| .into_iter() | |
| .map(|o| self.context_to_objs(o, context_chain, limitations)) | |
| { | |
| match res { | |
| Ok(mut objs) => all_objs.append(&mut objs), | |
| Err(_) => { | |
| // Swallow expected-miss errors; they do not cause | |
| // the overall lookup to fail. | |
| } | |
| } | |
| } | |
| Ok(all_objs) |
| `goto-implementations`) | ||
|
|
||
| ## Relevant environment variables | ||
| * `MAX_LOG_MESSAGE_LENGTH` When set, will truncate any outputted logs that are longer than the value. Defaults to 1000 bytes. Set to 0 to turn off truncation. |
There was a problem hiding this comment.
The docs say logs will be truncated to MAX_LOG_MESSAGE_LENGTH, but the implementation currently slices to max_len and then appends " ...", producing output longer than the configured value when truncation occurs. Please clarify the behavior here (either adjust docs to mention the suffix, or adjust truncation logic to keep the final output within the configured limit).
| * `MAX_LOG_MESSAGE_LENGTH` When set, will truncate any outputted logs that are longer than the value. Defaults to 1000 bytes. Set to 0 to turn off truncation. | |
| * `MAX_LOG_MESSAGE_LENGTH` When set, will truncate any outputted log messages that are longer than the value and append `" ..."`. Up to `MAX_LOG_MESSAGE_LENGTH` bytes of the original message are preserved; the suffix may cause the final output to slightly exceed this value. Defaults to 1000 bytes. Set to 0 to turn off truncation. |
Attempts & Testing of optimization of the device analysis