-
Notifications
You must be signed in to change notification settings - Fork 143
adding memory and disk usage stats to bench tests #591
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
base: main
Are you sure you want to change the base?
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
| * Get the index build time for a dataset | ||
| */ | ||
| public static Double getIndexBuildTime(String datasetName) { | ||
| return indexBuildTimes.get(datasetName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps clarify the unit here, such as getIndexBuildTimeMillis or getIndexBuildTimeSeconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I will add that.
| { | ||
| // Capture initial memory and disk state | ||
| var diagnostics = new io.github.jbellis.jvector.example.benchmarks.diagnostics.BenchmarkDiagnostics(getDiagnosticLevel()); | ||
| diagnostics.setMonitoredDirectory(testDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does import not work for this symbol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not recall why this is fully qualified but I will clean it up
|
|
||
| if (!snapshots.isEmpty()) { | ||
| if (!snapshots.isEmpty() && level != DiagnosticLevel.NONE) { | ||
| SystemMonitor.SystemSnapshot preSnapshot = snapshots.get(snapshots.size() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea for an improvement, DiagnosticLevel could control both snapshot collection and then iterate through the snapshots, even if there are zero. Just a simplification possibly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, will look into it
| * Utility class for monitoring disk usage during benchmark execution. | ||
| * Tracks total disk space used and number of files created. | ||
| */ | ||
| public class DiskUsageMonitor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly suggest using the notify mechanism that allows you to see update events within the filesystem, to avoid traversing things fully every time you want a read. Then you could easily keep a current view that only processes deltas for changes as they occur. This would possibly move some FS IO out of the measured bucket so that IO measurements are less perturbed by infra code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is great feedback. I'll hold off on this PR until I get that in place
jshook
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've approved this, but please see my comments regarding notify. I think you might have a better way to implement this which is well supported and much more efficient.
This PR adds new columns to the BenchYAML tests measuring memory and disk usage. The output looks as follows: