Review of dataflow package#1707
Conversation
| addToWorklistAgain); | ||
| } | ||
| } else { | ||
| S regular = inputBefore.copy().getRegularStore(); |
There was a problem hiding this comment.
Why do we need to do the copy() at all?
There was a problem hiding this comment.
We don't need it. getRegularStore() either computes a fresh leastUpperBound result (two-store case) or returns the store directly (single-store case). Since the analysis never mutates stores in-place — leastUpperBound/widenedUpperBound always return fresh stores, and the transfer function always works on a copy() of inputBefore — aliasing is safe. Removed in 12b976b.
| public static boolean listIsDeterministic( | ||
| List<? extends @Nullable JavaExpression> list, AnnotationProvider provider) { | ||
| return list.stream().allMatch(je -> je == null || je.isDeterministic(provider)); | ||
| for (int i = 0, n = list.size(); i < n; ++i) { |
There was a problem hiding this comment.
@copilot Assuming this is on a hot path, is it worth replacing lambdas by explicit loops to improve performance?
There was a problem hiding this comment.
The lambda at line 167 was already replaced with an explicit for loop in commit 3345ace (the "Avoid lambdas and re-computations" change). The current code avoids Stream creation and lambda overhead on this hot path.
Agent-Logs-Url: https://github.com/eisop/checker-framework/sessions/4154a81f-92cc-4710-b27d-e86ded904b80 Co-authored-by: wmdietl <6699136+wmdietl@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors multiple dataflow utilities to reduce allocations/overhead (replacing streams with loops, avoiding unnecessary map lookups/copies) and tightens some internal invariants around identity-based data structures and lazy copying in analysis results.
Changes:
- Simplifies purity queries and adds explicit handling for auto-generated record members.
- Replaces several stream/map patterns with loops and direct
get()usage to reduce overhead. - Refines lazy-copy behavior in
AnalysisResultand optimizes some CFG/dataflow lookups.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dataflow/src/main/java/org/checkerframework/dataflow/util/PurityUtils.java | Refactors deterministic/side-effect-free checks; special-cases record accessors. |
| dataflow/src/main/java/org/checkerframework/dataflow/expression/MethodCall.java | Replaces stream-based modifiability check with loops. |
| dataflow/src/main/java/org/checkerframework/dataflow/expression/JavaExpression.java | Replaces streams with loops and refactors varargs handling to reduce repeated calls. |
| dataflow/src/main/java/org/checkerframework/dataflow/constantpropagation/ConstantPropagationStore.java | Uses Map.get() instead of containsKey()+get() patterns. |
| dataflow/src/main/java/org/checkerframework/dataflow/cfg/ControlFlowGraph.java | Uses identity-based visited set for DFS order and minor lookup/loop refactors. |
| dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferInput.java | Adjusts hashCode() to align with equals() semantics. |
| dataflow/src/main/java/org/checkerframework/dataflow/analysis/ForwardAnalysisImpl.java | Attempts to reduce copying during exceptional successor propagation. |
| dataflow/src/main/java/org/checkerframework/dataflow/analysis/AnalysisResult.java | Splits lazy-copy tracking to avoid copying tree/postfix maps when only nodeValues may be mutated. |
| dataflow/src/main/java/org/checkerframework/dataflow/analysis/AbstractAnalysis.java | Avoids materializing full transitive operand sets by using an early-exit traversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
No description provided.