View Issue Details

IDProjectCategoryView StatusLast Update
0035302LazarusTAChartpublic2019-04-01 17:40
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60806 
Target Version2.2Fixed in Version 
Summary0035302: TAChart: some fixes for source's extent
DescriptionI'm attaching a patch, that fixes some issues in TListChartSource / TCustomChartSource, and also solves one serious problem.



Let's start from the serious problem. Let's consider an example in Example.png. The series' extent will be in this case: X = [1 .. 3] and Y = [101 .. 103]. To make further description simpler, we'll take a look only at Y range.

Once extent is calculated in TCustomChartSource.BasicExtent(), it is internally cached. TListChartSource inherits from TListChartSource, and implements some nice optimization: when we change some point's Y value, in most cases we don't need to invalidate the whole cached extent - it's enough only to modify it, if needed.

For example: we want to change second point's Y value - this is performed by calling TListChartSource.SetYValue():
a) from Y=102 to Y = 100 -> new Y is below the current cached extent -> so update the cached extent to be [100 .. 103]
b) from Y=102 to Y = 101 -> new Y is within the current cached extent -> no action required
c) from Y=102 to Y = 103 -> new Y is within the current cached extent -> no action required
d) from Y=102 to Y = 104 -> new Y is above the current cached extent -> so update the cached extent to be [101 .. 104]

Another example: we want to change first point's Y value:
e) from Y=101 to Y = 100 -> new Y is below the current cached extent -> so update the cached extent to be [100 .. 103]
f) from Y=101 to Y = 102 -> we removed cached extent's bottom bound -> so cached extent needs to be fully recalculated
g) from Y=101 to Y = 103 -> we removed cached extent's bottom bound -> so cached extent needs to be fully recalculated
h) from Y=101 to Y = 104 -> we removed cached extent's bottom bound -> so cached extent needs to be fully recalculated

The problem is in TListChartSource.SetXValue() and SetYValue() - more precisely in their internal UpdateExtent() procedures. For cases f), g) and h), UpdateExtent() immediately recalculates the whole extent. This causes a problem when we want to shift every data point by some value:

  for I := 0 to List.Count - 1 do
    List.SetYValue(I, I + 567);

In our example, this will be:
- I=0: we removed cached extent's bottom bound, i.e. 101 -> cached extent is fully recalculated, by iterating all the existing data points
- I=1: we removed cached extent's bottom bound, i.e. 102 -> cached extent is fully recalculated, by iterating all the existing data points
- I=2: we removed cached extent's bottom bound, i.e. 103 -> cached extent is fully recalculated, by iterating all the existing data points

This way, something that was intended to make things faster (by updating the cached extent, instead of invalidating it), slows us horribly down: we iterate through 3 data points 3 times, i.e. we have List.Count ^ 2 data point accesses - so for List.Count like 1000, we will get 1000000 data accesses.

The attached ExtentSpeedTest application shows this:
- execution time with "Lucky case" option (adding -123456 to every point's Y): 0 ms
- execution time with "Unlucky case" option (adding +123456 to every point's Y): 7600 ms

The solution is quite simple: instead of recalculating the cached extent each time, we should just invalidate it. Then it will be calculated again when needed, which will be already after modifying all the data points.

After applying the patch:
- execution time with "Lucky case" option: 0 ms
- execution time with "Unlucky case" option: 0 ms



More detailed changelog:



Changes in tacustomsource.pas:

1) There is a protected function TCustomChartSource.CalcExtentXYList(), which was added about five weeks ago - I added lacking check for YCount > 1 there. By the way, I added also "UseYList" parameter (currently only "UseXList" parameter is used) to provide identical handling also for Y values - maybe someone will find this useful when inheriting from TCustomChartSource.

2) In TCustomChartSource.ExtentCumulative(), upper array range is now taken directly from "High(YList)", instead of "YCount - 2" - which is more safe, and also as it is already in other places.

3) In TCustomChartSource.GetErrorBarValues(), range check for XCount has been fixed (as a reference: 10 lines below there is same check for YCount).



Changes in tasources.pas:

4) No longer needed, local SourceClassString() function has been removed.

5) In TListChartSource.Delete(), lacking checks for XCount / YCount have been added.

6) In TListChartSource.SetXCount() / SetYCount(), lacking calls to InvalidateCaches() have ben added to invalidate the cached extent; all the other existing TCustomChartSource descendants call InvalidateCaches() properly.

7) In TListChartSource.SetXCount() / SetYCount() -> UpdateExtent(), lacking checks for XCount / YCount have been added, and the problem described above has been fixed.
TagsNo tags attached.
Fixed in Revision60802-4,60809-16
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • Example.png (2,256 bytes)
    Example.png (2,256 bytes)
  • ExtentSpeedTest.zip (2,474 bytes)
  • patch.diff (10,403 bytes)
    Index: components/tachart/tacustomsource.pas
    ===================================================================
    --- components/tachart/tacustomsource.pas	(revision 60806)
    +++ components/tachart/tacustomsource.pas	(working copy)
    @@ -162,7 +162,7 @@
         function GetIndex(AIndex: Integer): Integer;
         function GetValue(AIndex: Integer): Double;
         function IsErrorBarValueStored(AIndex: Integer): Boolean;
    -    procedure SetKind(AValue: TChartErrorbarKind);
    +    procedure SetKind(AValue: TChartErrorBarKind);
         procedure SetIndex(AIndex, AValue: Integer);
         procedure SetValue(AIndex: Integer; AValue: Double);
       public
    @@ -193,7 +193,7 @@
         FValuesTotalIsValid: Boolean;
         FXCount: Cardinal;
         FYCount: Cardinal;
    -    function CalcExtentXYList(UseXList: Boolean): TDoubleRect;
    +    function CalcExtentXYList(UseXList, UseYList: Boolean): TDoubleRect;
         procedure ChangeErrorBars(Sender: TObject); virtual;
         function GetCount: Integer; virtual; abstract;
         function GetErrorBarValues(APointIndex: Integer; Which: Integer;
    @@ -742,7 +742,7 @@
       Result := FValue[AIndex];
     end;
     
    -function TChartErrorBarData.IsErrorbarValueStored(AIndex: Integer): Boolean;
    +function TChartErrorBarData.IsErrorBarValueStored(AIndex: Integer): Boolean;
     begin
       if AIndex = 0 then
         Result := FValue[0] <> 0
    @@ -820,9 +820,8 @@
       // empty
     end;
     
    -{ Calculates the extent including multiple x and/or y values (non-stacked)
    -  UseXList = true: consider both XList and YList, otherwise only YList. }
    -function TCustomChartSource.CalcExtentXYList(UseXList: Boolean): TDoubleRect;
    +{ Calculates the extent including multiple x and/or y values (non-stacked) }
    +function TCustomChartSource.CalcExtentXYList(UseXList, UseYList: Boolean): TDoubleRect;
     var
       i, j: Integer;
       jxp, jxn: Integer;
    @@ -830,6 +829,7 @@
     begin
       Result := Extent;
       if (YCount < 2) and (XCount < 2) then exit;
    +  if (not UseXList) and (not UseYList) then exit;
     
       // Skip the x and y values used for error bars when calculating the list extent.
       if UseXList and (XErrorBarData.Kind = ebkChartSource) then begin
    @@ -839,8 +839,8 @@
         jxp := -1;
         jxn := -1;
       end;
    -  if YErrorBarData.Kind = ebkChartSource then begin
    -    jyp := YErrorbarData.IndexPlus - 1;  // -1 because YList is offset by 1
    +  if UseYList and (YErrorBarData.Kind = ebkChartSource) then begin
    +    jyp := YErrorBarData.IndexPlus - 1;  // -1 because YList is offset by 1
         jyn := YErrorBarData.IndexMinus - 1;
       end else begin
         jyp := -1;
    @@ -854,13 +854,13 @@
               if (j <> jxp) and (j <> jxn) then
                 UpdateMinMax(XList[j], Result.a.X, Result.b.X);
           end;
    -  for i := 0 to Count - 1 do
    -    with Item[i]^ do begin
    -      for j := 0 to High(YList) do
    -        if (j <> jyp) and (j <> jyn) then
    -          UpdateMinMax(YList[j], Result.a.Y, Result.b.Y);
    -    end
    -
    +  if UseYList and (YCount > 1) then
    +    for i := 0 to Count - 1 do
    +      with Item[i]^ do begin
    +        for j := 0 to High(YList) do
    +          if (j <> jyp) and (j <> jyn) then
    +            UpdateMinMax(YList[j], Result.a.Y, Result.b.Y);
    +      end
     end;
     
     
    @@ -876,7 +876,7 @@
       inherited Create(AOwner);
       FXCount := 1;
       FYCount := 1;
    -  for i:=Low(FErrorBardata) to High(FErrorBarData) do begin
    +  for i:=Low(FErrorBarData) to High(FErrorBarData) do begin
         FErrorBarData[i] := TChartErrorBarData.Create;
         FErrorBarData[i].OnChange := @ChangeErrorBars;
       end;
    @@ -929,27 +929,28 @@
         jyn := YErrorBarData.IndexMinus - 1;
       end;
     
    -  for i := 0 to Count - 1 do begin
    -    h := NumberOr(Item[i]^.Y);
    -    for j := 0 to YCount - 2 do
    -      if (j <> jyp) and (j <> jyn) then begin
    -        h += NumberOr(Item[i]^.YList[j]);
    -        // If some of the Y values are negative, h may be non-monotonic.
    -        UpdateMinMax(h, Result.a.Y, Result.b.Y);
    -      end;
    -  end;
    +  for i := 0 to Count - 1 do
    +    with Item[i]^ do begin
    +      h := NumberOr(Y);
    +      for j := 0 to High(YList) do
    +        if (j <> jyp) and (j <> jyn) then begin
    +          h += NumberOr(YList[j]);
    +          // If some of the Y values are negative, h may be non-monotonic.
    +          UpdateMinMax(h, Result.a.Y, Result.b.Y);
    +        end;
    +    end;
     end;
     
     { Calculates the extent including multiple y values (non-stacked) }
     function TCustomChartSource.ExtentList: TDoubleRect;
     begin
    -  Result := CalcExtentXYList(false);
    +  Result := CalcExtentXYList(false, true);
     end;
     
     { Calculates the extent including multiple x and y values (non-stacked) }
     function TCustomChartSource.ExtentXYList: TDoubleRect;
     begin
    -  Result := CalcExtentXYList(true);
    +  Result := CalcExtentXYList(true, true);
     end;
     
     // ALB -> leftmost item where X >= AXMin, or Count if no such item
    @@ -1031,7 +1032,7 @@
       Result := TCustomChartSourceEnumerator.Create(Self);
     end;
     
    -function TCustomChartSource.GetErrorbarData(AIndex: Integer): TChartErrorBarData;
    +function TCustomChartSource.GetErrorBarData(AIndex: Integer): TChartErrorBarData;
     begin
       Result := FErrorBarData[AIndex];
     end;
    @@ -1081,7 +1082,7 @@
             if Which = 0 then begin
               pidx := FErrorBarData[0].IndexPlus;
               nidx := FErrorBarData[0].IndexMinus;
    -          if not InRange(pidx, 0, XCount) then exit;
    +          if not InRange(pidx, 0, XCount-1) then exit;
               if (nidx <> -1) and not InRange(nidx, 0, XCount-1) then exit;
               AUpperDelta := Item[APointIndex]^.GetX(pidx);
               if nidx = -1 then
    @@ -1152,7 +1153,7 @@
     
     function TCustomChartSource.GetHasErrorBars(Which: Integer): Boolean;
     var
    -  errbar: TChartErrorbarData;
    +  errbar: TChartErrorBarData;
     begin
       Result := false;
       errbar := FErrorBarData[Which];
    @@ -1212,10 +1213,10 @@
       Result := false;
     end;
     
    -procedure TCustomChartSource.SetErrorbarData(AIndex: Integer;
    +procedure TCustomChartSource.SetErrorBarData(AIndex: Integer;
       AValue: TChartErrorBarData);
     begin
    -  FErrorbarData[AIndex] := AValue;
    +  FErrorBarData[AIndex] := AValue;
       Notify;
     end;
     
    Index: components/tachart/tasources.pas
    ===================================================================
    --- components/tachart/tasources.pas	(revision 60806)
    +++ components/tachart/tasources.pas	(working copy)
    @@ -397,11 +397,6 @@
         p += 1;
       end;
     
    -  function SourceClassString: String;
    -  begin
    -    Result := IfThen(FSource.Name <> '', FSource.Name, FSource.ClassName);
    -  end;
    -
       function StrToFloatOrDateTime(const AStr: String): Double;
       begin
         if (AStr = '') or (AStr = '?') then
    @@ -411,7 +406,7 @@
              not TryStrToFloat(AStr, Result) and
              not TryStrToDateTime(AStr, Result)
           then
    -        raise EListSourceStringError.CreateFmt(rsListSourceNumericError, [SourceClassString, AStr]);
    +        raise EListSourceStringError.CreateFmt(rsListSourceNumericError, [NameOrClassName(FSource), AStr]);
         end;
       end;
     
    @@ -421,7 +416,7 @@
           Result := clTAColor
         else
         if not TryStrToInt(AStr, Result) then
    -      raise EListSourceStringError.CreateFmt(rsListSourceColorError, [SourceClassString, AStr]);
    +      raise EListSourceStringError.CreateFmt(rsListSourceColorError, [NameOrClassName(FSource), AStr]);
       end;
     
     var
    @@ -436,7 +431,7 @@
         // Text must be quoted if it contains '|'.
         if (Cardinal(parts.Count) <> FSource.XCount + FSource.YCount + 2) then
           raise EListSourceStringError.CreateFmt(
    -        rsListSourceStringFormatError, [SourceClassString, ChopString(AString, 20)]);
    +        rsListSourceStringFormatError, [NameOrClassName(FSource), ChopString(AString, 20)]);
     
         with ADataItem^ do begin
           if FSource.XCount > 0 then begin
    @@ -614,8 +609,8 @@
     begin
       with Item[AIndex]^ do begin
         FExtentIsValid := FExtentIsValid and
    -      (FExtent.a.X < X) and (X < FExtent.b.X) and
    -      (FExtent.a.Y < Y) and (Y < FExtent.b.Y);
    +      ((XCount = 0) or ((FExtent.a.X < X) and (X < FExtent.b.X))) and
    +      ((YCount = 0) or ((FExtent.a.Y < Y) and (Y < FExtent.b.Y)));
         if FValuesTotalIsValid then
           FValuesTotal -= NumberOr(Y);
       end;
    @@ -704,6 +699,7 @@
       FXCount := AValue;
       for i := 0 to Count - 1 do
         SetLength(Item[i]^.XList, Max(FXCount - 1, 0));
    +  InvalidateCaches;
       Notify;
     end;
     
    @@ -723,31 +719,18 @@
       oldX: Double;
     
       procedure UpdateExtent;
    -  var
    -    it: PChartDataItem;
       begin
    -    if not FExtentIsValid then exit;
    +    if (not FExtentIsValid) or (XCount = 0) then exit;
     
         if not IsNan(AValue) then begin
    -      if AValue <= FExtent.a.X then
    +      if AValue < FExtent.a.X then
             FExtent.a.X := AValue
    -      else if AValue >= FExtent.b.X then
    +      else if AValue > FExtent.b.X then
             FExtent.b.X := AValue;
         end;
     
    -    if IsNan(oldX) then exit;
    -    if oldX = FExtent.b.X then begin
    -      FExtent.b.X := NegInfinity;
    -      for it in Self do
    -        if not IsNan(it^.X) then
    -          FExtent.b.X := Max(FExtent.b.X, it^.X);
    -    end;
    -    if oldX = FExtent.a.X then begin
    -      FExtent.a.X := SafeInfinity;
    -      for it in Self do
    -        if not IsNan(it^.X) then
    -          FExtent.a.X := Min(FExtent.a.X, it^.X);
    -    end;
    +    if not IsNan(oldX) then
    +      FExtentIsValid := (oldX <> FExtent.a.X) and (oldX <> FExtent.b.X);
       end;
     
     begin
    @@ -781,6 +764,7 @@
       FYCount := AValue;
       for i := 0 to Count - 1 do
         SetLength(Item[i]^.YList, Max(FYCount - 1, 0));
    +  InvalidateCaches;
       Notify;
     end;
     
    @@ -799,31 +783,18 @@
       oldY: Double;
     
       procedure UpdateExtent;
    -  var
    -    it: PChartDataItem;
       begin
    -    if not FExtentIsValid then exit;
    +    if (not FExtentIsValid) or (YCount = 0) then exit;
     
         if not IsNan(AValue) then begin
    -      if AValue <= FExtent.a.Y then
    +      if AValue < FExtent.a.Y then
             FExtent.a.Y := AValue
    -      else if AValue >= FExtent.b.Y then
    +      else if AValue > FExtent.b.Y then
             FExtent.b.Y := AValue;
         end;
     
    -    if IsNan(oldY) then exit;
    -    if oldY = FExtent.b.Y then begin
    -      FExtent.b.Y := NegInfinity;
    -      for it in Self do
    -        if not IsNan(it^.Y) then
    -          FExtent.b.Y := Max(FExtent.b.Y, it^.Y);
    -    end;
    -    if oldY = FExtent.a.Y then begin
    -      FExtent.a.Y := SafeInfinity;
    -      for it in Self do
    -        if not IsNan(it^.Y) then
    -          FExtent.a.Y := Min(FExtent.a.Y, it^.Y);
    -    end;
    +    if not IsNan(oldY) then
    +      FExtentIsValid := (oldY <> FExtent.a.Y) and (oldY <> FExtent.b.Y);
       end;
     
     begin
    
    patch.diff (10,403 bytes)

Activities

Marcin Wiazowski

2019-03-31 21:50

reporter  

Example.png (2,256 bytes)
Example.png (2,256 bytes)

Marcin Wiazowski

2019-03-31 21:51

reporter  

ExtentSpeedTest.zip (2,474 bytes)

Marcin Wiazowski

2019-03-31 21:51

reporter  

patch.diff (10,403 bytes)
Index: components/tachart/tacustomsource.pas
===================================================================
--- components/tachart/tacustomsource.pas	(revision 60806)
+++ components/tachart/tacustomsource.pas	(working copy)
@@ -162,7 +162,7 @@
     function GetIndex(AIndex: Integer): Integer;
     function GetValue(AIndex: Integer): Double;
     function IsErrorBarValueStored(AIndex: Integer): Boolean;
-    procedure SetKind(AValue: TChartErrorbarKind);
+    procedure SetKind(AValue: TChartErrorBarKind);
     procedure SetIndex(AIndex, AValue: Integer);
     procedure SetValue(AIndex: Integer; AValue: Double);
   public
@@ -193,7 +193,7 @@
     FValuesTotalIsValid: Boolean;
     FXCount: Cardinal;
     FYCount: Cardinal;
-    function CalcExtentXYList(UseXList: Boolean): TDoubleRect;
+    function CalcExtentXYList(UseXList, UseYList: Boolean): TDoubleRect;
     procedure ChangeErrorBars(Sender: TObject); virtual;
     function GetCount: Integer; virtual; abstract;
     function GetErrorBarValues(APointIndex: Integer; Which: Integer;
@@ -742,7 +742,7 @@
   Result := FValue[AIndex];
 end;
 
-function TChartErrorBarData.IsErrorbarValueStored(AIndex: Integer): Boolean;
+function TChartErrorBarData.IsErrorBarValueStored(AIndex: Integer): Boolean;
 begin
   if AIndex = 0 then
     Result := FValue[0] <> 0
@@ -820,9 +820,8 @@
   // empty
 end;
 
-{ Calculates the extent including multiple x and/or y values (non-stacked)
-  UseXList = true: consider both XList and YList, otherwise only YList. }
-function TCustomChartSource.CalcExtentXYList(UseXList: Boolean): TDoubleRect;
+{ Calculates the extent including multiple x and/or y values (non-stacked) }
+function TCustomChartSource.CalcExtentXYList(UseXList, UseYList: Boolean): TDoubleRect;
 var
   i, j: Integer;
   jxp, jxn: Integer;
@@ -830,6 +829,7 @@
 begin
   Result := Extent;
   if (YCount < 2) and (XCount < 2) then exit;
+  if (not UseXList) and (not UseYList) then exit;
 
   // Skip the x and y values used for error bars when calculating the list extent.
   if UseXList and (XErrorBarData.Kind = ebkChartSource) then begin
@@ -839,8 +839,8 @@
     jxp := -1;
     jxn := -1;
   end;
-  if YErrorBarData.Kind = ebkChartSource then begin
-    jyp := YErrorbarData.IndexPlus - 1;  // -1 because YList is offset by 1
+  if UseYList and (YErrorBarData.Kind = ebkChartSource) then begin
+    jyp := YErrorBarData.IndexPlus - 1;  // -1 because YList is offset by 1
     jyn := YErrorBarData.IndexMinus - 1;
   end else begin
     jyp := -1;
@@ -854,13 +854,13 @@
           if (j <> jxp) and (j <> jxn) then
             UpdateMinMax(XList[j], Result.a.X, Result.b.X);
       end;
-  for i := 0 to Count - 1 do
-    with Item[i]^ do begin
-      for j := 0 to High(YList) do
-        if (j <> jyp) and (j <> jyn) then
-          UpdateMinMax(YList[j], Result.a.Y, Result.b.Y);
-    end
-
+  if UseYList and (YCount > 1) then
+    for i := 0 to Count - 1 do
+      with Item[i]^ do begin
+        for j := 0 to High(YList) do
+          if (j <> jyp) and (j <> jyn) then
+            UpdateMinMax(YList[j], Result.a.Y, Result.b.Y);
+      end
 end;
 
 
@@ -876,7 +876,7 @@
   inherited Create(AOwner);
   FXCount := 1;
   FYCount := 1;
-  for i:=Low(FErrorBardata) to High(FErrorBarData) do begin
+  for i:=Low(FErrorBarData) to High(FErrorBarData) do begin
     FErrorBarData[i] := TChartErrorBarData.Create;
     FErrorBarData[i].OnChange := @ChangeErrorBars;
   end;
@@ -929,27 +929,28 @@
     jyn := YErrorBarData.IndexMinus - 1;
   end;
 
-  for i := 0 to Count - 1 do begin
-    h := NumberOr(Item[i]^.Y);
-    for j := 0 to YCount - 2 do
-      if (j <> jyp) and (j <> jyn) then begin
-        h += NumberOr(Item[i]^.YList[j]);
-        // If some of the Y values are negative, h may be non-monotonic.
-        UpdateMinMax(h, Result.a.Y, Result.b.Y);
-      end;
-  end;
+  for i := 0 to Count - 1 do
+    with Item[i]^ do begin
+      h := NumberOr(Y);
+      for j := 0 to High(YList) do
+        if (j <> jyp) and (j <> jyn) then begin
+          h += NumberOr(YList[j]);
+          // If some of the Y values are negative, h may be non-monotonic.
+          UpdateMinMax(h, Result.a.Y, Result.b.Y);
+        end;
+    end;
 end;
 
 { Calculates the extent including multiple y values (non-stacked) }
 function TCustomChartSource.ExtentList: TDoubleRect;
 begin
-  Result := CalcExtentXYList(false);
+  Result := CalcExtentXYList(false, true);
 end;
 
 { Calculates the extent including multiple x and y values (non-stacked) }
 function TCustomChartSource.ExtentXYList: TDoubleRect;
 begin
-  Result := CalcExtentXYList(true);
+  Result := CalcExtentXYList(true, true);
 end;
 
 // ALB -> leftmost item where X >= AXMin, or Count if no such item
@@ -1031,7 +1032,7 @@
   Result := TCustomChartSourceEnumerator.Create(Self);
 end;
 
-function TCustomChartSource.GetErrorbarData(AIndex: Integer): TChartErrorBarData;
+function TCustomChartSource.GetErrorBarData(AIndex: Integer): TChartErrorBarData;
 begin
   Result := FErrorBarData[AIndex];
 end;
@@ -1081,7 +1082,7 @@
         if Which = 0 then begin
           pidx := FErrorBarData[0].IndexPlus;
           nidx := FErrorBarData[0].IndexMinus;
-          if not InRange(pidx, 0, XCount) then exit;
+          if not InRange(pidx, 0, XCount-1) then exit;
           if (nidx <> -1) and not InRange(nidx, 0, XCount-1) then exit;
           AUpperDelta := Item[APointIndex]^.GetX(pidx);
           if nidx = -1 then
@@ -1152,7 +1153,7 @@
 
 function TCustomChartSource.GetHasErrorBars(Which: Integer): Boolean;
 var
-  errbar: TChartErrorbarData;
+  errbar: TChartErrorBarData;
 begin
   Result := false;
   errbar := FErrorBarData[Which];
@@ -1212,10 +1213,10 @@
   Result := false;
 end;
 
-procedure TCustomChartSource.SetErrorbarData(AIndex: Integer;
+procedure TCustomChartSource.SetErrorBarData(AIndex: Integer;
   AValue: TChartErrorBarData);
 begin
-  FErrorbarData[AIndex] := AValue;
+  FErrorBarData[AIndex] := AValue;
   Notify;
 end;
 
Index: components/tachart/tasources.pas
===================================================================
--- components/tachart/tasources.pas	(revision 60806)
+++ components/tachart/tasources.pas	(working copy)
@@ -397,11 +397,6 @@
     p += 1;
   end;
 
-  function SourceClassString: String;
-  begin
-    Result := IfThen(FSource.Name <> '', FSource.Name, FSource.ClassName);
-  end;
-
   function StrToFloatOrDateTime(const AStr: String): Double;
   begin
     if (AStr = '') or (AStr = '?') then
@@ -411,7 +406,7 @@
          not TryStrToFloat(AStr, Result) and
          not TryStrToDateTime(AStr, Result)
       then
-        raise EListSourceStringError.CreateFmt(rsListSourceNumericError, [SourceClassString, AStr]);
+        raise EListSourceStringError.CreateFmt(rsListSourceNumericError, [NameOrClassName(FSource), AStr]);
     end;
   end;
 
@@ -421,7 +416,7 @@
       Result := clTAColor
     else
     if not TryStrToInt(AStr, Result) then
-      raise EListSourceStringError.CreateFmt(rsListSourceColorError, [SourceClassString, AStr]);
+      raise EListSourceStringError.CreateFmt(rsListSourceColorError, [NameOrClassName(FSource), AStr]);
   end;
 
 var
@@ -436,7 +431,7 @@
     // Text must be quoted if it contains '|'.
     if (Cardinal(parts.Count) <> FSource.XCount + FSource.YCount + 2) then
       raise EListSourceStringError.CreateFmt(
-        rsListSourceStringFormatError, [SourceClassString, ChopString(AString, 20)]);
+        rsListSourceStringFormatError, [NameOrClassName(FSource), ChopString(AString, 20)]);
 
     with ADataItem^ do begin
       if FSource.XCount > 0 then begin
@@ -614,8 +609,8 @@
 begin
   with Item[AIndex]^ do begin
     FExtentIsValid := FExtentIsValid and
-      (FExtent.a.X < X) and (X < FExtent.b.X) and
-      (FExtent.a.Y < Y) and (Y < FExtent.b.Y);
+      ((XCount = 0) or ((FExtent.a.X < X) and (X < FExtent.b.X))) and
+      ((YCount = 0) or ((FExtent.a.Y < Y) and (Y < FExtent.b.Y)));
     if FValuesTotalIsValid then
       FValuesTotal -= NumberOr(Y);
   end;
@@ -704,6 +699,7 @@
   FXCount := AValue;
   for i := 0 to Count - 1 do
     SetLength(Item[i]^.XList, Max(FXCount - 1, 0));
+  InvalidateCaches;
   Notify;
 end;
 
@@ -723,31 +719,18 @@
   oldX: Double;
 
   procedure UpdateExtent;
-  var
-    it: PChartDataItem;
   begin
-    if not FExtentIsValid then exit;
+    if (not FExtentIsValid) or (XCount = 0) then exit;
 
     if not IsNan(AValue) then begin
-      if AValue <= FExtent.a.X then
+      if AValue < FExtent.a.X then
         FExtent.a.X := AValue
-      else if AValue >= FExtent.b.X then
+      else if AValue > FExtent.b.X then
         FExtent.b.X := AValue;
     end;
 
-    if IsNan(oldX) then exit;
-    if oldX = FExtent.b.X then begin
-      FExtent.b.X := NegInfinity;
-      for it in Self do
-        if not IsNan(it^.X) then
-          FExtent.b.X := Max(FExtent.b.X, it^.X);
-    end;
-    if oldX = FExtent.a.X then begin
-      FExtent.a.X := SafeInfinity;
-      for it in Self do
-        if not IsNan(it^.X) then
-          FExtent.a.X := Min(FExtent.a.X, it^.X);
-    end;
+    if not IsNan(oldX) then
+      FExtentIsValid := (oldX <> FExtent.a.X) and (oldX <> FExtent.b.X);
   end;
 
 begin
@@ -781,6 +764,7 @@
   FYCount := AValue;
   for i := 0 to Count - 1 do
     SetLength(Item[i]^.YList, Max(FYCount - 1, 0));
+  InvalidateCaches;
   Notify;
 end;
 
@@ -799,31 +783,18 @@
   oldY: Double;
 
   procedure UpdateExtent;
-  var
-    it: PChartDataItem;
   begin
-    if not FExtentIsValid then exit;
+    if (not FExtentIsValid) or (YCount = 0) then exit;
 
     if not IsNan(AValue) then begin
-      if AValue <= FExtent.a.Y then
+      if AValue < FExtent.a.Y then
         FExtent.a.Y := AValue
-      else if AValue >= FExtent.b.Y then
+      else if AValue > FExtent.b.Y then
         FExtent.b.Y := AValue;
     end;
 
-    if IsNan(oldY) then exit;
-    if oldY = FExtent.b.Y then begin
-      FExtent.b.Y := NegInfinity;
-      for it in Self do
-        if not IsNan(it^.Y) then
-          FExtent.b.Y := Max(FExtent.b.Y, it^.Y);
-    end;
-    if oldY = FExtent.a.Y then begin
-      FExtent.a.Y := SafeInfinity;
-      for it in Self do
-        if not IsNan(it^.Y) then
-          FExtent.a.Y := Min(FExtent.a.Y, it^.Y);
-    end;
+    if not IsNan(oldY) then
+      FExtentIsValid := (oldY <> FExtent.a.Y) and (oldY <> FExtent.b.Y);
   end;
 
 begin
patch.diff (10,403 bytes)

wp

2019-04-01 00:20

developer   ~0115156

I did not yet look at it in detail, but it is documented (http://wiki.lazarus.freepascal.org/TAChart_documentation#List_source) that writing to TChartDataItem record elements by means of the Set* procedures triggers the notifications, direct access is much faster. Doing this in your demo, I get 0 ms, too, without any other changes.

procedure TForm1.ButtonTestClick(Sender: TObject);
var
  T1, T2: Double;
  Delta: Integer;
  List: TListChartSource;
  I: Integer;
begin
  if RadioButton1.Checked then
    Delta := -123456
  else
    Delta := 123456;

  List := TListChartSource.Create(nil);
  try
    for I := 0 to 10000 do
      List.Add(I, I);

    Caption:='...';
    T1:=Now;

    for I := 0 to List.Count - 1 do
      List.Item[I]^.X += Delta; // <---------- direct access
    for I := 0 to List.Count - 1 do
      List.Item[i]^.Y += Delta; // <---------- direct access

    List.Extent;

    T2:=Now;
    Caption:='Measured time: '+IntToStr(Round((T2-T1)*24*3600*1000))+' ms';
  finally
    List.Free;
  end;
end;

Marcin Wiazowski

2019-04-01 01:19

reporter   ~0115157

Indeed, you are right. However, the problem with large delays is not due to notifications. It is due to code, that was intended to make things faster, but makes them much slower - which is located in UpdateExtent() procedures:


function TListChartSource.SetXValue():

  procedure UpdateExtent;
  begin
    ....

    if IsNan(oldX) then exit;
    if oldX = FExtent.b.X then begin
      FExtent.b.X := NegInfinity;
      for it in Self do
        if not IsNan(it^.X) then
          FExtent.b.X := Max(FExtent.b.X, it^.X);
    end;
    if oldX = FExtent.a.X then begin
      FExtent.a.X := SafeInfinity;
      for it in Self do
        if not IsNan(it^.X) then
          FExtent.a.X := Min(FExtent.a.X, it^.X);
    end;
  end;


As we can see, "for it in Self do" enumerations are made on all data points. This can be faster than invalidating the whole cached extent only in one case: when we want to modify exactly one data point. This is because the code enumerates only X coordinates:

  FExtent.b.X := Max(FExtent.b.X, it^.X);
or
  FExtent.a.X := Min(FExtent.a.X, it^.X);

while invalidating the cached extent leads to enumerating both X and Y coordinates.


When we modify two data points, time spent in UpdateExtent() will be roughly equal to invalidating the whole cached extent.


When modifying three or more data points, invalidating the whole cached extent is always the best solution.



So, in all cases - except when modifying only one data point - applying the patch will make the execution faster - for large data sets even much, much faster.

And when modifying only one data point, additional execution time will be measured in microseconds.

So we sometimes waste some microseconds, but - in most cases - save up to seconds.

wp

2019-04-01 14:37

developer   ~0115159

Applied, thanks.

Marcin Wiazowski

2019-04-01 17:40

reporter   ~0115166

Thanks!

Issue History

Date Modified Username Field Change
2019-03-31 21:50 Marcin Wiazowski New Issue
2019-03-31 21:50 Marcin Wiazowski File Added: Example.png
2019-03-31 21:51 Marcin Wiazowski File Added: ExtentSpeedTest.zip
2019-03-31 21:51 Marcin Wiazowski File Added: patch.diff
2019-04-01 00:20 wp Note Added: 0115156
2019-04-01 00:21 wp Assigned To => wp
2019-04-01 00:21 wp Status new => assigned
2019-04-01 01:19 Marcin Wiazowski Note Added: 0115157
2019-04-01 14:37 wp Fixed in Revision => 60802-4,60809-16
2019-04-01 14:37 wp LazTarget => 2.2
2019-04-01 14:37 wp Note Added: 0115159
2019-04-01 14:37 wp Status assigned => resolved
2019-04-01 14:37 wp Resolution open => fixed
2019-04-01 14:37 wp Target Version => 2.2
2019-04-01 17:40 Marcin Wiazowski Note Added: 0115166
2019-04-01 17:40 Marcin Wiazowski Status resolved => closed