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

Fix GC stats. #48

Merged
merged 4 commits into from
Aug 6, 2024
Merged

Fix GC stats. #48

merged 4 commits into from
Aug 6, 2024

Conversation

kayceesrk
Copy link
Collaborator

Use wall clock time for per-domain time calculation. This is better than using CPU time.

If a core spends 98 s doing io, 1 s doing computation and 1 s doing GC, the GC overhead should be 1%.

@kayceesrk kayceesrk requested a review from Sudha247 July 26, 2024 03:47
Copy link
Member

@Sudha247 Sudha247 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks correct.

I'm not sure of the source of failure in the CI [link], but it doesn't look like the issue in #27.

let wall_time = { start_time = 0.; end_time = 0. }
let total_cpu_time = ref 0.

let domain_cpu_times = Array.make 128 (0.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 128 still the maximum number of domains we can create at a time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

128 is still the default. There is an option in 5.3 to increase the number of domains with the help of an OCAMLRUNPARAM parameter: #48. However, there is no way to get this number out programmatically. Perhaps there should be a Domain.max_domain_count similar to Domain.recommended_domain_count. Once we have that, we can replace 128 with the result of that.

lib/olly_gc_stats/olly_gc_stats.ml Outdated Show resolved Hide resolved
Use wall clock time for per-domain time calculation.
@kayceesrk
Copy link
Collaborator Author

Needs more fixes. We should be using the timestamp from the event not the one using Unix.gettimeofday.

@kayceesrk kayceesrk marked this pull request as ready for review July 27, 2024 13:25
@kayceesrk kayceesrk merged commit 57250e0 into tarides:main Aug 6, 2024
4 checks passed
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.

2 participants