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

Estimate the start time by variance #360

Merged

Conversation

kkdlau
Copy link
Contributor

@kkdlau kkdlau commented Jun 6, 2024

This PR is for fixing #257

Below is a summary of the changes made in this pull request:

  • New OnlineCalculator classes - WelfordVarianceCalculator and OnlineStatisticsCalculator
    • This two classes allows to compute the mean and variance on-the-fly, without caching the dataset in memory
  • Test cases for OnlineCalculator classes
  • Update the way of computing estimated start time

Regarding the estimated start time, I used a slightly different approach than suggested in #257

  • GC frequency is calculated as the time gap between two consecutive GC events.
    • For example, if event A happens at 2s and event B happens at 2.3s, the GC frequency is 2.3s - 2s = 0.3s.
  • If timeOfFirstEvent does not have time stamp, the method will return date stamp - SD of GC frequency
  • If timeOfFirstEvent has time stamp
  • it will first compute time stamp - SD of GC frequency
    • if the value is greater than 0, then return value directly
    • otherwise return timeOfFirstEvent, since we cannot have negative time stamp

Please review the code and provide any comments or feedback you may have. I would also appreciate clarification on the specific points mentioned above!

@kkdlau kkdlau changed the title WIP: Estimate the start time by variance (#257) WIP: Estimate the start time by variance Jun 6, 2024
@kkdlau
Copy link
Contributor Author

kkdlau commented Jun 6, 2024

@microsoft-github-policy-service agree

@karianna karianna requested review from kcpeppe and dsgrieve June 10, 2024 01:03
@karianna
Copy link
Member

Hi @kkdlau - @kcpeppe sees this as a good change :-), are you able to go ahead and also update the tests to match?

@kkdlau
Copy link
Contributor Author

kkdlau commented Jun 17, 2024

Hi @kkdlau - @kcpeppe sees this as a good change :-), are you able to go ahead and also update the tests to match?

definitely! Will push the change later

@kkdlau kkdlau changed the title WIP: Estimate the start time by variance Estimate the start time by variance Jun 20, 2024

}

@Aggregates({EventSource.CMS_PREUNIFIED,EventSource.CMS_UNIFIED,EventSource.G1GC,EventSource.GENERATIONAL,EventSource.JVM,EventSource.SHENANDOAH, EventSource.TENURED,EventSource.ZGC})
@Aggregates({EventSource.GENERATIONAL,EventSource.CMS_UNIFIED,EventSource.G1GC,EventSource.GENERATIONAL,EventSource.JVM,EventSource.SHENANDOAH, EventSource.TENURED,EventSource.ZGC})
Copy link
Member

Choose a reason for hiding this comment

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

This does seem incorrect? It's a duplicate?

@kcpeppe kcpeppe merged commit 81b2744 into microsoft:main Jun 24, 2024
8 checks passed
@karianna karianna mentioned this pull request Jul 3, 2024
@karianna karianna mentioned this pull request Oct 1, 2024
@karianna karianna mentioned this pull request Dec 8, 2024
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.

4 participants