MCTS.jl icon indicating copy to clipboard operation
MCTS.jl copied to clipboard

JET-suggested improvements and benchmarking with PkgBenchmarks

Open BoZenKhaa opened this issue 10 months ago • 1 comments

JET suggested improvements and benchmarking with PkgBenchmarks

This PR is a work in progress, and I am opening it for posterity since I have to move on for now and possibly to get other opinions on how to do this better.

I have tried using JET on MCTS.jl; this PR addresses issues highlighted by JET. I tried to benchmark the individual changes to see whether these issues were real.

Comparison of consecutive changes to baseline

The baseline should correspond to the latest master, with the addition of the benchmarking scripts.

Below are the results; the ratios are all to the same baseline, and all changes are additive, i.e., 30242a78 includes all the changes above it in the table.

# hash time ratio memory ratio comment
1 f6a815db 1.00 1.00 Baseline.
2 98450a1c 1.15 1.00 removed mutable where not needed, exposed sizehint externally.
3 b10aa869 1.21 0.98 Add type to array creation in insert_node.
4 30c18da2 1.08 0.98 Replace 'nothing' with empty tree - remove uniontype in planner.
5 76d103ee 1.03 0.41 Removed further abstract types from struct definitions.
6 9c421b29 1.01 0.41 Immutable SolvedRolloutEstimator.
7 30242a78 1.02 0.41 Removed abstract type from MCTSSolver struct.

The time regressions are minor; some of them could be noise. The memory improvement is quite significant, but I haven't investigated which part of the commit is responsible yet.

Todos and benchmarking

I fumbled a bit with the benchmarking. First, I tried using BenchmarkTools.jl manually, then I tried using AirspeedVelocity.jl until finally settling on using PkgBenchmark.jl which eventually worked out with following workflow:

  1. edit/create benchmarks in /benchmark/benchmarks.jl and commit them to a benchmarking branch
  2. git rebase the branch with commits I want to evaluate on top of the changes to the benchmark
  3. ] dev MCTS in the benchmarking environment, use PkgBenchmark.jl to evaluate the commits.

What's still missing:

  • I haven't checked what's causing the improvement in the memory
  • the benchmarking script is not part of the repo; it is only attached below
  • the initial commits contain code that the current master replaces; I should clean that up
  • JET suggested changes to RolloutSimulator in POMDPs.jl are not part of this benchmark
  • possibly, PkgBenchmarks could be included as part of the CI

benchmark_packages.zip

BoZenKhaa avatar Apr 18 '24 13:04 BoZenKhaa

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.58%. Comparing base (e6a4944) to head (30242a7).

Files Patch % Lines
src/vanilla.jl 76.47% 4 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage   86.74%   86.58%   -0.17%     
==========================================
  Files          11       11              
  Lines         483      492       +9     
==========================================
+ Hits          419      426       +7     
- Misses         64       66       +2     

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

codecov[bot] avatar Apr 18 '24 13:04 codecov[bot]