Improve performances of offsets references. #2481
Conversation
DarkaMaul
requested review from
montyly,
0xalpharush and
smonicas
as code owners
WalkthroughWalkthroughThe recent changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SlitherCore
participant SourceMapping
User->>SlitherCore: Call offset_to_objects()
SlitherCore->>SlitherCore: _get_offset()
SlitherCore-->>User: Return Set[SourceMapping]
User->>SlitherCore: Call offset_to_references()
SlitherCore->>SlitherCore: _get_offset()
SlitherCore-->>User: Return Set[SourceMapping]
User->>SlitherCore: Call offset_to_implementations()
SlitherCore->>SlitherCore: _get_offset()
SlitherCore-->>User: Return Set[SourceMapping]
User->>SlitherCore: Call offset_to_definitions()
SlitherCore->>SlitherCore: _get_offset()
SlitherCore-->>User: Return Set[SourceMapping]
This diagram visualizes the interaction between the user and the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- slither/core/slither_core.py (4 hunks)
- slither/core/source_mapping/source_mapping.py (1 hunks)
Additional context used
Ruff
slither/core/source_mapping/source_mapping.py
168-168: Ambiguous variable name:
l(E741)
170-170: Ambiguous variable name:
l(E741)slither/core/slither_core.py
516-517: Use a single
ifstatement instead of nestedifstatements (SIM102)
Additional comments not posted (1)
slither/core/slither_core.py (1)
91-91: The addition of new attributes and methods for handling offset mappings in theSlitherCoreclass is well-implemented and should contribute to the performance improvements as described in the PR.Also applies to: 204-255, 313-365
Verification successful
The new attributes and methods for handling offset mappings in the
SlitherCoreclass are properly integrated into the existing codebase. The search results show that_get_offsetand_offset_to_min_offsetare used in various parts of theslither_core.pyfile, indicating proper usage and integration.
- slither/core/slither_core.py: Multiple lines show the usage of
_offset_to_min_offsetand_get_offset.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of new offset handling methods with existing functionalities. # Test: Search for usage of new methods and attributes. Expect: Proper integration without issues. rg --type python $'_get_offset|_offset_to_min_offset'Length of output: 85
Script:
#!/bin/bash # Description: Verify integration of new offset handling methods with existing functionalities. # Test: Search for usage of new methods and attributes. Expect: Proper integration without issues. rg '_get_offset|_offset_to_min_offset'Length of output: 1214
| @@ -112,12 +112,8 @@ def __eq__(self, other: Any) -> bool: | |||
| try: | |||
| return ( | |||
| self.start == other.start | |||
| and self.length == other.length | |||
| and self.filename == other.filename | |||
| and self.filename.relative == other.filename.relative | |||
There was a problem hiding this comment.
The simplification of the __eq__ method in the Source class looks good and aligns with the PR's goal to improve performance.
Would you like me to help create unit tests to ensure this new behavior works as expected?
The problem
When trying to fix the
unused-importdetector problem, I noticed that the detector was kind of slow.The profiler showed that running it on the
contracts-bedrocktook about 22 seconds :The main culprit here is the
__compute_offsets_from_thingmethod.I noticed that the data is stored at each offset for every mapping in
slither_core:It was also running all checks within the loop, even if they did not depend on the loop offset.
The solution
offset_to_*mappings and created a new mappingoffset_to_min_offsetthat contains the referenced min_offset. This one is only frominttoSet[int], so it's reasonably compact and fast._get_offsetSourceMappingequality function to reduce its complexityThe results
We are going down from 22 seconds to about 14s so about 35% performance improvement.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes