Skip to content

Don't crash on pauses longer than 10sec#93

Open
ngorogiannis wants to merge 2 commits into
tarides:mainfrom
ngorogiannis:refactor-common
Open

Don't crash on pauses longer than 10sec#93
ngorogiannis wants to merge 2 commits into
tarides:mainfrom
ngorogiannis:refactor-common

Conversation

@ngorogiannis

@ngorogiannis ngorogiannis commented Jun 11, 2026

Copy link
Copy Markdown
  • Extract common code between 5.0 and 5.3 implementations. A modest reduction in code duplication. A more sophisticated refactor is possible at the cost of more complexity at the top levels.

  • Don't crash on pauses longer than 10sec. Done by adding a little bit of machinery to track such pauses and report some basic stats on them.

The main motivation is that it is possible, though not easily reproducible, to have pauses for more than 10sec (observed with workload described in #63 ). This crashes olly currently.

@ngorogiannis ngorogiannis changed the title Extract common code between 5.0 and 5.3 implementations Don't crash on pauses longer than 10sec Jun 11, 2026

@edwintorok edwintorok left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The common code extraction is best reviewed with git diff configured like this (unfortunately the UI doesn't provide this option):

[diff]
    algorithm = histogram
    colorMoved = dimmed_zebra
    colorMovedWS = allow-indentation-change
    renames = copies

With those settings the code in the 1st commit is grey (i.e. moved without changing), except for some whitespace which can be ignored, confirming that no typos slipped in.

Extracting the common code will be useful if we need to add another 5.x there in the future.

How long do you think we should maintain the 5.0-5.2 code though?
With 5.5 released should we consider dropping it?

Comment thread lib/olly_gc_stats/olly_gc_stats_common.ml Outdated
Comment thread lib/olly_gc_stats/olly_gc_stats.5.3.ml Outdated
@ngorogiannis

Copy link
Copy Markdown
Author

Extracting the common code will be useful if we need to add another 5.x there in the future.

How long do you think we should maintain the 5.0-5.2 code though? With 5.5 released should we consider dropping it?

That's a good question, CC @tmcgilchrist who introduced the split originally.

@tmcgilchrist

Copy link
Copy Markdown
Collaborator

I want to release a bug fix version that addresses the various attach, timeout and dropped events (as best as we can). Then we could think about dropping 5.0.

In general I think we should support the last 5 major releases and have a graceful fallback when we can't support newer features from runtime events.

@tmcgilchrist tmcgilchrist left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The refactor moved latency into the shared module but only switched gc_stats over to record_latency. Both subcommands build the same histogram from the same runtime_begin/runtime_end logic over the same GC phases, so the exact crash this PR is fixing still reproduces via olly latency.

lib/olly_gc_stats/olly_gc_stats_common.ml:161 still reads:

  let latency = Int64.to_int (Int64.sub (Ts.to_int64 ts) saved_ts) in
  assert (H.record_value hist latency)

Also this line has the recording call inside the assert, a build with -noassert compiles the call away entirely and the histogram silently avoids being updated.

The max_latency in JSON output is no longer the maximum value. Consider emitting max (H.max hist) outliers.max, or documenting the exclusion clearly.

The 10-second threshold isn't the real threshold. With the histogram's actual parameters (lowest_discernible_value:10, significant_figures:3, highest_trackable_value:10_000_000_000), bucket rounding means H.record_value accepts everything up to 17,179,869,183 ns and only starts rejecting at 2³⁴ ns.

olly_gc_stats_common.ml:140 hardcodes 10_000_000_000 instead of using the new highest_trackable_value constant defined above.

Could you add a CHANGELOG entry for this PR.

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.

3 participants