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

Add or migrate to peakAllocated #277

Open
Rosuavio opened this issue Jul 19, 2023 · 1 comment
Open

Add or migrate to peakAllocated #277

Rosuavio opened this issue Jul 19, 2023 · 1 comment

Comments

@Rosuavio
Copy link

It feels like It would be useful to have a more granular (or absolute) measurement of the "Peak Allocated".

It seems like we can either change peakMbAllocated to peakAllocated or add a peakAllocated along side peakMbAllocated.

For pre 4.10.0 we fill in the imprecision with zeros, and for 4.10.0 on we use the precise bytes.

@RyanGlScott
Copy link
Member

The only reason that we use megabytes in the first place is for backwards-compatibility with pre-4.10 versions of base that use this convention (see the old GCStats data type). But rather than add a separate field for peak allocated bytes, I wonder if we should instead migrate from criterion's GCStatistics data type (which attempts to offer backwards compatibility) in favor of base's RTSStats data type. RTSStats is the original source of truth for measurements in the first place, and its max_mem_in_use_bytes field is exactly what we need.

Of course, we would need to figure out an appropriate migration strategy. To introduce the use of RTSStats, we can follow a similar strategy as in #149, which migrated from GCStats to GCStatistics. We would also need to figure out what to do with the peakMbAllocated accessor in criterion. Should we remove it outright in favor of a different accessor that returns bytes instead of megabytes? Should we deprecate it by somehow emitting a warning whenever someone uses peakMbAllocated?

We'd also want to consider doing this for other fields of RTSStats that are more precise than their GCStats counterparts. For example, mutatorCpuSeconds currently uses seconds (another historical limitation imposed by GCStats), but RTSStats now measures the same information in nanoseconds.

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

No branches or pull requests

2 participants