View Issue Details

ID Project Category View Status Date Submitted Last Update 0035031 Lazarus TAChart public 2019-02-07 02:23 2019-02-15 01:04 Marcin Wiazowski wp normal minor always closed fixed 2.1 (SVN) 2.2 0035031: TAChart: mark drawing can be up to 40% faster 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 No tags attached. 60367, 60424 2.0.2 Win32/Win64 Attached Files

Activities

 2019-02-07 02:23 reporter test.diff (3,636 bytes)    ```Index: components/tachart/tacustomseries.pas =================================================================== --- components/tachart/tacustomseries.pas (revision 60353) +++ components/tachart/tacustomseries.pas (working copy) @@ -262,7 +262,7 @@ FOnCustomDrawPointer: TSeriesPointerCustomDrawEvent; FOnGetPointerStyle: TSeriesPointerStyleEvent; function GetErrorBars(AIndex: Integer): TChartErrorBar; - function GetLabelDirection(AIndex: Integer): TLabelDirection; + function GetLabelDirection(AIndex: Integer; const ACachedExtent: TDoubleRect): TLabelDirection; function IsErrorBarsStored(AIndex: Integer): Boolean; procedure SetErrorBars(AIndex: Integer; AValue: TChartErrorBar); procedure SetMarkPositionCentered(AValue: Boolean); @@ -1260,7 +1260,6 @@ y, ysum: Double; g: TDoublePoint; i, si: Integer; - ld: TLabelDirection; style: TChartStyle; lfont: TFont; curr, prev: Double; @@ -1283,21 +1282,12 @@ prev := GetZeroLevel else prev := TDoublePointBoolArr(ext.a)[not IsRotated]; - ld := GetLabelDirection(i); for si := 0 to Source.YCount - 1 do begin - if Styles <> nil then begin - style := Styles.StyleByIndex(si); - if style.UseFont then - Marks.LabelFont.Assign(style.Font) - else - Marks.LabelFont.Assign(lfont); - end; g := GetLabelDataPoint(i, si); if si > 0 then begin y := NumberOr(Source[i]^.YList[si-1], 0); - if IsNaN(y) then Continue; if Stacked then begin - if IsNaN(ysum) then ysum := y else ysum += y; + ysum += y; y := ysum; end; end; @@ -1320,8 +1310,16 @@ if ((Marks.YIndex = MARKS_YINDEX_ALL) or (Marks.YIndex = si)) and IsPointInViewPort(g) - then - DrawLabel(FormattedMark(i, '', si), GraphToImage(g), ld); + then begin + if Styles <> nil then begin + style := Styles.StyleByIndex(si); + if style.UseFont then + Marks.LabelFont.Assign(style.Font) + else + Marks.LabelFont.Assign(lfont); + end; + DrawLabel(FormattedMark(i, '', si), GraphToImage(g), GetLabelDirection(i, ext)); + end; end; end; @@ -1416,11 +1414,11 @@ Result := GetGraphPoint(AIndex); end; -function TBasicPointSeries.GetLabelDirection(AIndex: Integer): TLabelDirection; +function TBasicPointSeries.GetLabelDirection(AIndex: Integer; const ACachedExtent: TDoubleRect): TLabelDirection; function CenterLevel: Double; begin - with Extent do + with ACachedExtent do if IsRotated then Result := (b.x + a.x) * 0.5 else @@ -1806,6 +1804,7 @@ m: array [TLabelDirection] of Integer absolute AMargins; gp: TDoublePoint; scMarksDistance: Integer; + ext: TDoubleRect; begin if not Marks.IsMarkLabelsVisible or not Marks.AutoMargins then exit; if Count = 0 then exit; @@ -1823,6 +1822,7 @@ Count-1} FindExtentInterval(ParentChart.CurrentExtent, Source.IsSorted); + ext := Extent; scMarksDistance := ADrawer.Scale(Marks.Distance); for i := FLoBound to FUpBound do begin gp := GetGraphPoint(i); @@ -1830,7 +1830,7 @@ labelText := FormattedMark(i); if labelText = '' then continue; - dir := GetLabelDirection(i); + dir := GetLabelDirection(i, ext); with Marks.MeasureLabel(ADrawer, labelText) do dist := IfThen(dir in [ldLeft, ldRight], cx, cy); if Marks.DistanceToCenter then ``` test.diff (3,636 bytes) 2019-02-07 02:23 reporter SpeedTest.zip (3,748 bytes) 2019-02-08 22:03 developer   ~0113961 Thank you. Applied with modifications. Brought me also to a bug of mis-positioned series marks when the axes are rotated. 2019-02-08 23:17 reporter   ~0113964 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? 2019-02-08 23:52 developer   ~0113965 Last edited: 2019-02-08 23:57View 2 revisions 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. 2019-02-09 01:34 reporter   ~0113966 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. 2019-02-10 20:55 developer   ~0114020 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) 2019-02-12 00:45 reporter   ~0114044 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. 2019-02-12 00:45 reporter Reproduce.zip (3,795 bytes) 2019-02-12 00:46 reporter Reproduce.png (11,747 bytes)    Reproduce.png (11,747 bytes) 2019-02-13 17:48 reporter   ~0114092 "I found some issue for stacked line series, when StackedNaN = snDoNotDraw [...]" This is probably related: 0035077 2019-02-14 23:09 developer   ~0114129 > 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. 2019-02-15 01:04 reporter   ~0114131 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!

Issue History

2019-02-07 02:23 Marcin Wiazowski New Issue
2019-02-07 02:23 Marcin Wiazowski File Added: test.diff
2019-02-07 02:23 Marcin Wiazowski File Added: SpeedTest.zip
2019-02-07 09:52 wp Assigned To => wp
2019-02-07 09:52 wp Status new => assigned
2019-02-08 22:03 wp Fixed in Revision => 60367
2019-02-08 22:03 wp LazTarget => 2.0.2
2019-02-08 22:03 wp Note Added: 0113961
2019-02-08 22:03 wp Status assigned => resolved
2019-02-08 22:03 wp Resolution open => fixed
2019-02-08 22:03 wp Target Version => 2.0.2
2019-02-08 23:17 Marcin Wiazowski Note Added: 0113964
2019-02-08 23:17 Marcin Wiazowski Status resolved => assigned
2019-02-08 23:17 Marcin Wiazowski Resolution fixed => reopened
2019-02-08 23:52 wp Note Added: 0113965
2019-02-08 23:57 wp Note Edited: 0113965 View Revisions
2019-02-09 01:34 Marcin Wiazowski Note Added: 0113966
2019-02-10 20:55 wp Note Added: 0114020
2019-02-10 20:55 wp Status assigned => resolved
2019-02-10 20:55 wp Resolution reopened => fixed
2019-02-12 00:45 Marcin Wiazowski Note Added: 0114044
2019-02-12 00:45 Marcin Wiazowski Status resolved => assigned
2019-02-12 00:45 Marcin Wiazowski Resolution fixed => reopened
2019-02-12 00:45 Marcin Wiazowski File Added: Reproduce.zip
2019-02-12 00:46 Marcin Wiazowski File Added: Reproduce.png
2019-02-13 17:48 Marcin Wiazowski Note Added: 0114092
2019-02-14 23:09 wp Fixed in Revision 60367 => 60367, 60424
2019-02-14 23:09 wp Note Added: 0114129
2019-02-14 23:09 wp Status assigned => resolved
2019-02-14 23:09 wp Resolution reopened => fixed
2019-02-14 23:09 wp Target Version 2.0.2 => 2.2
2019-02-15 01:04 Marcin Wiazowski Note Added: 0114131
2019-02-15 01:04 Marcin Wiazowski Status resolved => closed