jacodb icon indicating copy to clipboard operation
jacodb copied to clipboard

Added cache for `jcClass`

Open Damtev opened this issue 2 years ago • 9 comments

Damtev avatar Dec 08 '23 14:12 Damtev

Lifecycle test results

48 tests  ±0   48 :heavy_check_mark: ±0   1m 19s :stopwatch: -6s   5 suites ±0     0 :zzz: ±0    5 files   ±0     0 :x: ±0 

Results for commit d584c0a8. ± Comparison against base commit 207c3067.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Dec 08 '23 14:12 github-actions[bot]

Test results on JDK 19

1 270 tests  ±0   1 259 :heavy_check_mark: ±0   4m 4s :stopwatch: -13s      45 suites ±0        11 :zzz: ±0       45 files   ±0          0 :x: ±0 

Results for commit d584c0a8. ± Comparison against base commit 207c3067.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Dec 08 '23 14:12 github-actions[bot]

Test results on JDK 8

1 270 tests  ±0   1 257 :heavy_check_mark: ±0   4m 11s :stopwatch: -1s      45 suites ±0        13 :zzz: ±0       45 files   ±0          0 :x: ±0 

Results for commit d584c0a8. ± Comparison against base commit 207c3067.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Dec 08 '23 14:12 github-actions[bot]

Test results on JDK 11

1 270 tests  ±0   1 261 :heavy_check_mark: ±0   4m 51s :stopwatch: -1s      45 suites ±0          9 :zzz: ±0       45 files   ±0          0 :x: ±0 

Results for commit d584c0a8. ± Comparison against base commit 207c3067.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Dec 08 '23 14:12 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.04%. Comparing base (207c306) to head (d584c0a). Report is 10 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #207      +/-   ##
=============================================
+ Coverage      77.01%   77.04%   +0.02%     
  Complexity      1632     1632              
=============================================
  Files            166      166              
  Lines           9543     9543              
  Branches        1693     1693              
=============================================
+ Hits            7350     7352       +2     
+ Misses          1513     1508       -5     
- Partials         680      683       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 08 '23 14:12 codecov[bot]

@Damtev are there any proves that this change increases performance?

  • Types has less time to live period
  • request to find class by name is almost always hits class cache
  • JcClass is huge object and will be nice to decouple types and classes. In this PR decopling process will be broken because holding reference to JcClassType almost always will hold link to JcClassOrInterface

lehvolk avatar Dec 13 '23 12:12 lehvolk

@lehvolk jcClass is used in calculating the hashCode of the JcClassType, and we had met a situation when the hashCode of an instance of the JcClassType has changed. We assumed that happens because the jcClass getter sometimes returns different instances of JcClassOrInterface, which is quite unexpected for users.

Damtev avatar Dec 14 '23 09:12 Damtev

@lehvolk jcClass is used in calculating the hashCode of the JcClassType, and we had met a situation when the hashCode of an instance of the JcClassType has changed.

This is quite strange because JcClassOrInterface designed to have proper equals method. Only case which I can imagine is that the same class located into two different jars. If you check if name and generics substitution is that same then only possible difference may occur into AbstractByteCodeLocation#jarOrFolder file

lehvolk avatar Dec 14 '23 11:12 lehvolk

@Damtev my concern is that this fix will hide a problem or will not fix the problem.

lehvolk avatar Dec 14 '23 15:12 lehvolk