Closed ennerf closed 1 year ago
Hey there! 👋 I've summarized the previous results for you. Here's a markdown document for your pull request review:
pom.xml
file at line 99.DurationMeasurement
variables in the Chart.java
file at lines 105-118.ensureJavaFxPulse()
in the Chart.java
file at lines 124-137.getCssMetaData()
in the Chart.java
file at lines 143-148.XYChartCss.java
file.AbstractAxis.java
file at lines 6-9.Profileable
interface in the AbstractAxis.java
file at line 11.DurationMeasurement
variables in the AbstractAxis.java
file at lines 32-35.setProfiler()
in the AbstractAxis.java
file at lines 123-134.DataSet
parameter from the updateLegend()
method in the Legend.java
file.Chart.java
, line 100, change protected final Pane canvasForeground = FXUtils.createUnmanagedPane();
to protected final Pane canvasForeground = StyleUtil.addStyles(new FullSizePane(), "chart-canvas-foreground");
for consistent styling.Chart.java
, line 164, change protected void invalidated() {
to protected void invalidated() {
for better code readability.Chart.java
, line 268, change var canvasArea = StyleUtil.addStyles(new PlotAreaPane(canvas, canvasForeground,
...updateDataSetIndices()
to update the global indices of the datasets.rendererChanged()
method to call updateDataSetIndices()
after adding or removing a renderer.datasetsChanged()
method to call updateDataSetIndices()
after setting the global indices.updateLegend()
method to accept only the list of renderers instead of both datasets and renderers.forEachDataSet()
method.Chart.java
, line 19, instead of commenting 'Update data ranges', it would be better to use a more descriptive comment like 'Update data ranges and trigger layout updates'.Chart.java
, line 28, instead of commenting 'Make sure the datasets won't be modified', it would be better to use a more descriptive comment like 'Lock the datasets to prevent modification during layout updates'.Chart.java
, line 36, instead of commenting 'Update data ranges etc. to trigger anything that might need a layout', it would be better to use a more descriptive comment like 'Update data ranges and trigger layout updates for components that depend on them'.Chart.java
, line 47, instead of commenting 'Update other components', it would be better to use a more descriptive comment like 'Update other components (renderers, plugins) before layout'.Chart.java
, line 57, instead of commenting 'nothing to do', it would be better to use a more descriptive comment like 'No layout updates required if the state...Please provide a rating from 0 to 10 based on the following criteria:
Feel free to add a brief explanation for each criterion.
That's it! Good luck with your pull request! 🚀
By the way, have you heard about our premium plan? It's perfect for analyzing big pull requests like this one. Just thought you might be interested! 😉
Patch coverage: 42.42%
and project coverage change: -0.45%
:warning:
Comparison is base (
38f34f6
) 48.51% compared to head (38c4a47
) 48.06%.:exclamation: Current head 38c4a47 differs from pull request most recent head 34f690b. Consider uploading reports for the commit 34f690b to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
After review I think that the measurements are benchmarks rather than profiling info, so I changed the names to reflect that.
Example
var recorder = MeasurementRecorders.newLiveDisplay("RollingBufferSample").matches(".*(all).*");
beamIntensityRenderer.setRecorder(recorder.contains("all").addPostfix("error"));
The PR also adds includes a BasicDataSetRenderer
that is more efficient for rendering standard datasets without errors. It is based on a renderer that we use internally.
There are also a few classes like CircularDoubleDataSet2D
and PercentileAxis
that are currently package private that should maybe become public at some point.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
35 Code Smells
No Coverage information
0.0% Duplication
This PR builds on top of #603 (dataset styling)
It looks like there were some profiling capabilities in the
ProcessingProfiler
, but it seems limited to logs and I don't know whether there is any tool to visualize it.Interfaces
I've been thinking a bit about this, and I think I came up with a nice interface that is extremely flexible while also providing very fine grained access to items of interest. It currently lives in the dataset module, but once it's stable I'd like to turn the base into a separate project so it's usable in non-chart projects as well. It consists of 3 interfaces:
Profiler
is something that creates named measurementsDurationMeasure
can measure time by using start and stop. An appropriate clock is chosen internally, but there is also a fallback to record a raw value for cases where timestamps are passed around (e.g. in event sourcing timestamps are often kept with the events)Profileable
interfaceEach metric of interest gets a dedicated measure. The field initialize to a
DISABLED
implementation with empty methods that should be able to be removed by dead code elimination.Examples
Debug printer
The simplest profiler is a debug printer that prints start/stop tags and some timing information to a log.
Tag selection
Via default methods we can create a fluent API for filtering tags of interest and modify the tags with meta-information for better disambiguation.
HdrHistogram output
HdrHistogram is an established and very low-overhead way to measure production behavior in low-latency environments (banking, real-time systems etc.). It records latencies in high-dynamic range histograms (i.e. resolution depends on the absolute value, so it's very space efficient) and provides a file format that can be read by various tools. I've used it a few times, so I had some code sitting around.
The profiler records histograms and continuously writes them to disk in set intervals. The histograms are stored with a tag so they can be disambiguated again later. Profiling an axis and loading it in HdrHistogramVisualizer (a tool I wrote a long time ago when testing the normal JavaFX Charts) looks as below
The top chart shows the maxima for each interval, and the lower chart shows a percentile plot with an exponential x axis.
Live chart
Given that this is a charting library, I figured it'd be appropriate to also have a live chart of the real-time generated profiling data:
I already had some code for HdrHistogram-like visualizations, so I put together an initial demo of it profiling itself. Here is a video.