Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JET-suggested improvements and benchmarking with PkgBenchmarks #108

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

BoZenKhaa
Copy link
Contributor

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., 30242a7 includes all the changes above it in the table.

# hash time ratio memory ratio comment
1 f6a815d 1.00 1.00 Baseline.
2 98450a1 1.15 1.00 removed mutable where not needed, exposed sizehint externally.
3 b10aa86 1.21 0.98 Add type to array creation in insert_node.
4 30c18da 1.08 0.98 Replace 'nothing' with empty tree - remove uniontype in planner.
5 76d103e 1.03 0.41 Removed further abstract types from struct definitions.
6 9c421b2 1.01 0.41 Immutable SolvedRolloutEstimator.
7 30242a7 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 BoZenKhaa marked this pull request as draft April 18, 2024 13:06
Copy link

codecov bot commented Apr 18, 2024

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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant