Skip to content

eflatency: compute the mean from the timings array#240

Closed
osresearch wants to merge 1 commit intoXilinx-CNS:masterfrom
osresearch:eflatency-mean
Closed

eflatency: compute the mean from the timings array#240
osresearch wants to merge 1 commit intoXilinx-CNS:masterfrom
osresearch:eflatency-mean

Conversation

@osresearch
Copy link
Copy Markdown

This patch changes how the mean column in eflatency is computed to use the sum of the per-message timings, instead of the overall time that the test used. That way things that are outside of the timing loop do not affect the computation of the mean and the mean value reflects the min/50/90/95/max values in the other columns.

For most users this will result in a slight decrease of their mean times, although ideally it should be very close for the default case.

This patch changes how the mean column in eflatency is computed
to use the sum of the per-message timings, instead of the
overall time that the test used.  That way things that are outside
of the timing loop do not affect the computation of the mean and
the mean value reflects the min/50/90/95/max values in the other
columns.

For most users this will result in a slight decrease of their mean
times, although ideally it should be very close for the default case.
@ivatet-amd
Copy link
Copy Markdown
Contributor

Thank you @osresearch.

Although I understand the motivation, I don't think we can apply this patch because it might confuse the users and give a wrong impression of the performance improvement since the mean would decrease.

Meanwhile, I'll let @jfeather-amd follow up at #238.

@osresearch
Copy link
Copy Markdown
Author

I see that a similar change has been merged in the v9.0.0 branch in yaml output mode dd61f46 so this can be closed.

@osresearch osresearch closed this Oct 11, 2024
@pemberso-xilinx
Copy link
Copy Markdown
Contributor

I see that a similar change has been merged in the v9.0.0 branch in yaml output mode dd61f46 so this can be closed.

Thanks for the initial suggestion @osresearch . If you want to raise any other PR building on dd61f46 , we're happy to take a look.

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