| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0035031 | Lazarus | TAChart | public | 2019-02-07 03:23 | 2019-02-15 02:04 |
| Reporter | Marcin Wiazowski | Assigned To | wp | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | 2.1 (SVN) | Product Build | 60353 | ||
| Target Version | 2.2 | Fixed in Version | |||
| Summary | 0035031: TAChart: mark drawing can be up to 40% faster | ||||
| Description | Mark drawing can be made faster by making some optimizations; the most noticeable speedup is for multi-value data sources. In procedure TBasicPointSeries.DrawLabels(): - a call to GetLabelDirection() is made in every loop iteration - although it may be done when really needed, i.e. just before the DrawLabel() call - a "Styles" handling is made in every iteration of the inner loop - although it may be done when really needed, i.e. just before the DrawLabel() call - there is: y := NumberOr(Source[i]^.YList[si-1], 0); if IsNaN(y) then Continue; the second line can be removed, since NumberOr() guarantees, that "y" is never NaN - there is: if IsNan(Source[i]^.Point) then continue; y := Source[i]^.Y; <==== y is never NaN ysum := y; <==== ysum is never NaN ... y := NumberOr(Source[i]^.YList[si-1], 0); <==== y is never NaN ... if IsNaN(ysum) then ysum := y else ysum += y; <==== see below "ysum" cannot become NaN when initialized, and later it can be only incremented by "y", which is also guaranteed not to be NaN, so - in fact - it can never become NaN. So calling "IsNaN(ysum)" always returns False. So "if IsNaN(ysum) then ysum := y else ysum += y;" can be changed just to "ysum += y;" And the most important optimization: the TBasicPointSeries.GetLabelDirection() method uses a TBasicPointSeries.Extent() function - which makes a call to data source's ExtentCumulative() or ExtentList() method. They are very fast for NON multi-value sources (since extent is cached), and horribly slow for multi-value sources. Unfortunately, TBasicPointSeries.GetLabelDirection() is called in loops, so - for multi-value sources - this makes a large slowdown. Solution: Since GetLabelDirection() is strict private, an additional parameter can be added to pass an extent - that has been earlier saved to variable, before the loop. The attached test program is artificial - it uses stacked, multi-valued line series, but only marks are visible - no pointers or lines. But this is just to test the hardest code path at once, and not to be influenced by line or pointer drawing times. What's more, on a multitasking operating system, measuring time difference like 10 ms is not very reliable - but a difference of 1000 ms can be trusted in; to force longer testing times, a lot of data points are drawn on the chart. Times measured on a low-end laptop: without patch about 3370 ms, with patch about 1860 ms. Regards | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | 60367, 60424 | ||||
| LazTarget | 2.0.2 | ||||
| Widgetset | Win32/Win64 | ||||
| Attached Files |
| ||||
|
|
test.diff (3,636 bytes) |
|
|
SpeedTest.zip (3,748 bytes) |
|
|
Thank you. Applied with modifications. Brought me also to a bug of mis-positioned series marks when the axes are rotated. |
|
|
Fix working as expected here. It seems that two potential improvement have been missed. I have also one doubt: 1) in TBasicPointSeries.DrawLabels(), you can remove the "ld" variable and move the "GetLabelDirection(i, centerLvl)" call directly as a parameter to the "DrawLabel(...)" call below 2) the declaration: function GetLabelDirection(AIndex: Integer; ACenterLevel: Double): TLabelDirection; may be changed to: function GetLabelDirection(AIndex: Integer; const ACenterLevel: Double): TLabelDirection; since passing arguments larger than 4 bytes (32-bit code) or 8 bytes (64-bit) code may generate more optimal code when using "const" or "var" declaration (because a pointer to data may be passed, instead of copying the data to the stack) 3) in TBasicPointSeries.DrawLabels(), there is a "yIsNaN" variable. It is initialized before a loop, and then updated in every loop iteration (assuming that si > 0). So, effectively, "yIsNaN" is telling if THE LAST Y VALUE (for given X) is NaN. Below an "and (not yIsNaN)" condition is checked, so it effectively checks if THE LAST Y VALUE is NaN. Is this intentional? |
|
|
I'll fix 1) and 2) when I am finished with my current work. 3) This is supposed to avoid painting marks to missing values of y multi-valued series and adding subsequent y values to NaN. However, I recently noticed that the += operator does not crash when the right operand is NaN, so this may change. The entire routine may change again also for another reason, and maybe you could give me your advice here: In TAChart, a missing value is coded as a NaN value. In non-stacked series missing values are displayed as breaks in a line and area series and as a missing bar in a bar series. But what about stacked series? When one y value somewhere in the stack is missing how will the following stack levels be plotted because their values strictly speaking cannot be added any more? Excel replaces them by 0, and that's how I modified the Barseries (that's why the yIsNaN thing came in). What is your opinion? Should missing values in stacked series be replaced by 0, or should the entire datapoint be omitted? Or should only the levels below the one with the NaN value be painted? The more I think about it I favor the solution to drop the entire data point when the x or at least one y value is missing. All others are somehow misleading. |
|
|
I think that you have three good ideas: 1) "[...] missing values in stacked series be replaced by 0" 2) "[...] the entire datapoint be omitted" 3) "[...] only the levels below the one with the NaN value be painted" You could make the behavior just configurable (by some new property). I can imagine some situations, when these options could be usable: ad 1) Every day (your "X") you count people visiting a cinema, having 4 gates (doors). One day, one of the employees was sick, so you could only count people at 3 gates (Y1, Y2 and Y4); the gate with no employee receives Y3 = Nan in this case (not 0, because 0 would mean that visitors have been counted, but nobody used this gate). In this case, Y3 = NaN should be replaced with 0 in the chart's bar, and Y1, Y2 and Y4 should still be used for painting - so the bar still shows a *guaranteed* number of visitors (i.e. the best that we can do in this situation). ad 2) Every day (your "X") you read data from 4 flow sensors, each showing amount of water used by a production line (for cooling). One day, one of the flow sensors failed to work (so Y3 = NaN). In this case, you may wish to ignore also results form the other 3 sensors, that are still working properly (Y1, Y2 and Y4) - i.e. display nothing (empty bar) on the chart - otherwise someone could thing, that the water usage was unexpectedly low that day. ad 3) Well, I don't have any clever example here - but I'm sure that there exist many cases ideal for this option. Which of these options should be a default one? Option 1) is most intuitive, since many chart users are also Excel users - and also this is the only possible behavior now. Option 2) is not a good choice - not seeing anything is not very helpful when designing, in particular in IDE. Option 3) is somewhere between 1) and 2). So I would vote for 1) as a default setting. |
|
|
Pos (1) and (2) are included in r60402 along with other changes. GetLabeldirection needs a revisit because it mixes up graph and axis coordinates and does not consider the values of stacked series correctly, but only the original values from the series. As for the stacked missing values, I implemented options 1) and 2) so far, 1) is default. They correspond to a new property StackedNaN = (snReplaceByZero, snDoNotDraw) |
|
|
Well, I can see that you worked hard last days. 1) and 2) are fixed, 3) is as designed. I found some issue for stacked line series, when StackedNaN = snDoNotDraw. When one of Y values is NaN, marks are not drawn at all, but all data points below Y = NaN - and their connecting lines - are still drawn. Please see the attached image and the application. |
|
|
Reproduce.zip (3,795 bytes) |
|
|
Reproduce.png (11,747 bytes) |
|
|
"I found some issue for stacked line series, when StackedNaN = snDoNotDraw [...]" This is probably related: 0035077 |
|
|
> I found some issue for stacked line series, when StackedNaN = snDoNotDraw. When > one of Y values is NaN, marks are not drawn at all, but all data points below > Y = NaN - and their connecting lines - are still drawn. Fixed. |
|
|
Fix confirmed, everything works properly now - thanks! BTW: Thanks to solving a problem described in 0035077 (where each mark has been drawn multiple times in case of using a multi-value data source), execution time of the attached SpeedTest application has been dramatically reduced - from about 1860 ms to about 610 ms here! |
