Skip to content

Conversation

@clayrat
Copy link
Contributor

@clayrat clayrat commented Jun 6, 2023

  • Add a plot-performance Haskell script to flexibly generate perf charts without relying on gnuplot (currently based on a fork of haskell-chart, will be switched back to Hackage when a new version is released)
  • Update corresponding section in README
  • Fix codebase links in README
  • Minor refactor of the plot-benchmarks script

@clayrat clayrat changed the title minor refactor of benchmark-timings Better performance plotting script and readme updates Jun 6, 2023
@clayrat
Copy link
Contributor Author

clayrat commented Jun 6, 2023

Here are the charts produced by the proposed script from data generated for #2173:

filtered
top
bot

@facundominguez
Copy link
Collaborator

Thanks @clayrat! I imagine that producing the summaries of top speedups/slowdowns motivates part of the PR. Are there other benefits that you are seeking with it?

@clayrat
Copy link
Contributor Author

clayrat commented Jun 6, 2023

Thanks @clayrat! I imagine that producing the summaries of top speedups/slowdowns motivates part of the PR. Are there other benefits that you are seeking with it?

Mainly the ability to deal with added/removed tests separately and not having to rotate the charts by 90 degrees manually to have readable test names :) In general I think it will be easier to extend a Haskell app with further customizations if we ever need them (e.g. various label styles for failing tests etc) rather than trying to figure out corresponding gnuplot commands.

@facundominguez
Copy link
Collaborator

I gave it a try today but it seems I can't get plot-performance to produce a chart with all of the tests. @clayrat, could you share a chart showing the whole testsuite?

@clayrat
Copy link
Contributor Author

clayrat commented Jun 7, 2023

I gave it a try today but it seems I can't get plot-performance to produce a chart with all of the tests. @clayrat, could you share a chart showing the whole testsuite?

@facundominguez Do you mean you want a plot with all 1198 tests? Currently, the script has a hardwired viewport size of 2048 x 2048 (as in the original) and the rendering engine resizes the bar width to fit all data points uniformly, with a bottom cap. Under these constraints, at approximately 190 data points, the bars start overlapping and with further increase, the engine also starts dropping some labels to keep the remainder readable. A straightforward workaround for this would be to add an optional parameter to customize the plot height. Here's the plot of all tests with a size of 2048 x 16384, I'm not sure however such plots are very practical to use:

perf

@facundominguez
Copy link
Collaborator

Currently, the script has a hardwired viewport size of 2048 x 2048 (as in the original)

Ah, I see. Now that there would be top speed ups and slow downs, a chart of the whole test suite is less of a need.

The change looks good to adopt and try. I have some usability concerns that I share below. But let me know if you would like to leave these for another PR.

It would be preferable to set the height of the viewport based on how many rows need to be displayed. An alternative is to print a warning when there are too many rows, but this is less than ideal because then the user needs to rerun the script and figure out how to change the invocation.

As multiple charts might be produced now, it would be helpful to have the tool print their names to stdout so the user knows immediately which files have been written.

When using -o, maybe the script should try creating the directory if it is missing to save the user the trouble of creating it.

@clayrat
Copy link
Contributor Author

clayrat commented Jun 8, 2023

@facundominguez I think I've addressed your points 2 and 3. As for the dynamic viewport height (point 1), it seems to be a non-trivial task to compute it exactly, so for now I've added a simple case-based heuristic to decide on an appropriate power of 2, using values that work on my system. Feel free to lower the thresholds if they don't work for you (e.g. due to varying font sizes).

@nikivazou nikivazou requested a review from facundominguez June 8, 2023 16:42
Copy link
Collaborator

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

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

Feel free to lower the thresholds if they don't work for you (e.g. due to varying font sizes).

Thanks @clayrat. This is not a request to do further changes, but something we could try later. Would it make sense to assume a maximum height for each row, so we can compute the viewport height linearly regardless of what the font height is exactly?

I think this is good to merge as is. I might try contributing later on some simplifications to the code and explaining it a bit in comments. But feel free to polish it further if you have ideas.

@nikivazou nikivazou merged commit 39d7899 into ucsd-progsys:develop Jun 9, 2023
@clayrat
Copy link
Contributor Author

clayrat commented Jun 9, 2023

Would it make sense to assume a maximum height for each row, so we can compute the viewport height linearly regardless of what the font height is exactly?

Sure, as a first approximation, you can only use the bar widths, that parameter is set here: https://github.com/ucsd-progsys/liquidhaskell/blob/develop/scripts/plot-performance/app/Plot.hs#L50 The problem is that, as I said, the rendering engine will also start dropping labels if they overlap, which in our case would be the test names on the Y axis. So ideally you need to consider both bar sizes and font sizes to keep everything nice and readable.

@clayrat clayrat deleted the plot-performance branch June 9, 2023 17:25
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