View Issue Details

IDProjectCategoryView StatusLast Update
0035333LazarusTAChartpublic2019-04-07 22:50
ReporterMarcin WiazowskiAssigned Towp 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN)Product Build60857 
Target Version2.2Fixed in Version 
Summary0035333: TAChart: simple improvement for data sources
DescriptionI'm attaching a patch, that makes adding data to a source about 15% faster in some cases, just for free.


The attached test application creates a new TListChartSource object, and then measures execution time of the following sequence:

  List.BeginUpdate;
  try
    for I := 1 to 2000000 do
      List.Add(I,I);
  finally
    List.EndUpdate;
  end;
  List.Extent;

The problem is that, initially, the List object has its caches (in particular extent caches) valid. Because caches are valid, each Add() call checks, if - after adding new data - caches are still coherent with the data; this consumes time.

But, at the end, an EndUpdate() call is made, which invalidates all the caches unconditionally... This is required, because application is allowed to modify data directly between BeginUpdate() and EndUpdate() calls. But this also means, that all the efforts, made by Add() calls to keep caches coherent, are just wasted...


The solution is quite simple: caches should be invalidated not only in an EndUpdate() call, but also in a BeginUpdate() call. This will save us from all the efforts, performed to keep caches coherent between BeginUpdate() and EndUpdate() calls.


The attached test application gives results like:
- without the attached patch: 484 ms
- with the patch applied: 406 ms
TagsNo tags attached.
Fixed in Revision60868
LazTarget2.2
WidgetsetWin32/Win64
Attached Files
  • SpeedTest.zip (2,227 bytes)
  • patch.diff (1,042 bytes)
    Index: components/tachart/tacustomsource.pas
    ===================================================================
    --- components/tachart/tacustomsource.pas	(revision 60857)
    +++ components/tachart/tacustomsource.pas	(working copy)
    @@ -219,6 +219,7 @@
       public
         procedure AfterDraw; virtual;
         procedure BeforeDraw; virtual;
    +    procedure BeginUpdate; override;
         procedure EndUpdate; override;
       public
         class procedure CheckFormat(const AFormat: String);
    @@ -930,6 +931,17 @@
       Notify;
     end;
     
    +procedure TCustomChartSource.BeginUpdate;
    +begin
    +  // Caches will be eventually invalidated in a corresponding EndUpdate() call.
    +  // Since, at this moment, we are already sure, that caches will be invalidated,
    +  // it's better to invalidate them immediately - this will prevent useless efforts
    +  // to keep caches coherent between BeginUpdate() and EndUpdate() calls.
    +  if FUpdateCount = 0 then
    +    InvalidateCaches;
    +  Inc(FUpdateCount);
    +end;
    +
     procedure TCustomChartSource.EndUpdate;
     begin
       Dec(FUpdateCount);
    
    patch.diff (1,042 bytes)
  • patch_ver2.diff (1,966 bytes)
    Index: components/tachart/tacustomsource.pas
    ===================================================================
    --- components/tachart/tacustomsource.pas	(revision 60865)
    +++ components/tachart/tacustomsource.pas	(working copy)
    @@ -219,6 +219,7 @@
       public
         procedure AfterDraw; virtual;
         procedure BeforeDraw; virtual;
    +    procedure BeginUpdate; override;
         procedure EndUpdate; override;
       public
         class procedure CheckFormat(const AFormat: String);
    @@ -930,6 +931,17 @@
       Notify;
     end;
     
    +procedure TCustomChartSource.BeginUpdate;
    +begin
    +  // Caches will be eventually invalidated in a corresponding EndUpdate() call.
    +  // Since, at this moment, we are already sure, that caches will be invalidated,
    +  // it's better to invalidate them immediately - this will prevent useless efforts
    +  // to keep caches coherent between BeginUpdate() and EndUpdate() calls.
    +  if FUpdateCount = 0 then
    +    InvalidateCaches;
    +  Inc(FUpdateCount);
    +end;
    +
     procedure TCustomChartSource.EndUpdate;
     begin
       Dec(FUpdateCount);
    Index: components/tachart/tasources.pas
    ===================================================================
    --- components/tachart/tasources.pas	(revision 60865)
    +++ components/tachart/tasources.pas	(working copy)
    @@ -612,6 +612,13 @@
     
     procedure TListChartSource.Delete(AIndex: Integer);
     begin
    +  // Optimization
    +  if IsUpdating then begin
    +    Dispose(Item[AIndex]);
    +    FData.Delete(AIndex);
    +    exit;
    +  end;
    +
       with Item[AIndex]^ do begin
         FBasicExtentIsValid := FBasicExtentIsValid and
           (((FBasicExtent.a.X < X) and (X < FBasicExtent.b.X)) or (XCount = 0)) and
    @@ -873,6 +880,7 @@
     
     procedure TListChartSource.UpdateCachesAfterAdd(AX, AY: Double);
     begin
    +  if IsUpdating then exit; // Optimization
       if FBasicExtentIsValid then begin
         if FXCount > 0 then UpdateMinMax(AX, FBasicExtent.a.X, FBasicExtent.b.X);
         if FYCount > 0 then UpdateMinMax(AY, FBasicExtent.a.Y, FBasicExtent.b.Y);
    
    patch_ver2.diff (1,966 bytes)

Activities

Marcin Wiazowski

2019-04-07 02:07

reporter  

SpeedTest.zip (2,227 bytes)

Marcin Wiazowski

2019-04-07 02:07

reporter  

patch.diff (1,042 bytes)
Index: components/tachart/tacustomsource.pas
===================================================================
--- components/tachart/tacustomsource.pas	(revision 60857)
+++ components/tachart/tacustomsource.pas	(working copy)
@@ -219,6 +219,7 @@
   public
     procedure AfterDraw; virtual;
     procedure BeforeDraw; virtual;
+    procedure BeginUpdate; override;
     procedure EndUpdate; override;
   public
     class procedure CheckFormat(const AFormat: String);
@@ -930,6 +931,17 @@
   Notify;
 end;
 
+procedure TCustomChartSource.BeginUpdate;
+begin
+  // Caches will be eventually invalidated in a corresponding EndUpdate() call.
+  // Since, at this moment, we are already sure, that caches will be invalidated,
+  // it's better to invalidate them immediately - this will prevent useless efforts
+  // to keep caches coherent between BeginUpdate() and EndUpdate() calls.
+  if FUpdateCount = 0 then
+    InvalidateCaches;
+  Inc(FUpdateCount);
+end;
+
 procedure TCustomChartSource.EndUpdate;
 begin
   Dec(FUpdateCount);
patch.diff (1,042 bytes)

wp

2019-04-07 17:03

developer   ~0115303

Aren't there some more places to optimize?

- TListChartSource.SetXValue's UpdateExtent can be exited immediately if "IsUpdating" is true.

- dto with .SetYValue; moreover, updating its ValuesTotal can be skipped if "IsUpdating" is true.

- .UpdateCachesAfterAdd can be exited immediately if "IsUpdating" is true.

Marcin Wiazowski

2019-04-07 18:08

reporter  

patch_ver2.diff (1,966 bytes)
Index: components/tachart/tacustomsource.pas
===================================================================
--- components/tachart/tacustomsource.pas	(revision 60865)
+++ components/tachart/tacustomsource.pas	(working copy)
@@ -219,6 +219,7 @@
   public
     procedure AfterDraw; virtual;
     procedure BeforeDraw; virtual;
+    procedure BeginUpdate; override;
     procedure EndUpdate; override;
   public
     class procedure CheckFormat(const AFormat: String);
@@ -930,6 +931,17 @@
   Notify;
 end;
 
+procedure TCustomChartSource.BeginUpdate;
+begin
+  // Caches will be eventually invalidated in a corresponding EndUpdate() call.
+  // Since, at this moment, we are already sure, that caches will be invalidated,
+  // it's better to invalidate them immediately - this will prevent useless efforts
+  // to keep caches coherent between BeginUpdate() and EndUpdate() calls.
+  if FUpdateCount = 0 then
+    InvalidateCaches;
+  Inc(FUpdateCount);
+end;
+
 procedure TCustomChartSource.EndUpdate;
 begin
   Dec(FUpdateCount);
Index: components/tachart/tasources.pas
===================================================================
--- components/tachart/tasources.pas	(revision 60865)
+++ components/tachart/tasources.pas	(working copy)
@@ -612,6 +612,13 @@
 
 procedure TListChartSource.Delete(AIndex: Integer);
 begin
+  // Optimization
+  if IsUpdating then begin
+    Dispose(Item[AIndex]);
+    FData.Delete(AIndex);
+    exit;
+  end;
+
   with Item[AIndex]^ do begin
     FBasicExtentIsValid := FBasicExtentIsValid and
       (((FBasicExtent.a.X < X) and (X < FBasicExtent.b.X)) or (XCount = 0)) and
@@ -873,6 +880,7 @@
 
 procedure TListChartSource.UpdateCachesAfterAdd(AX, AY: Double);
 begin
+  if IsUpdating then exit; // Optimization
   if FBasicExtentIsValid then begin
     if FXCount > 0 then UpdateMinMax(AX, FBasicExtent.a.X, FBasicExtent.b.X);
     if FYCount > 0 then UpdateMinMax(AY, FBasicExtent.a.Y, FBasicExtent.b.Y);
patch_ver2.diff (1,966 bytes)

Marcin Wiazowski

2019-04-07 18:09

reporter   ~0115304

Nice observations.


After applying the patch, all the caches are invalidated in the BeginUpdate() call, which sets:

  FBasicExtentIsValid := false;
  FValuesTotalIsValid := false;



- In TListChartSource.SetXValue(), in UpdateExtent(), we have:

    if (not FBasicExtentIsValid) or (...) then exit;

which means, that there is - in fact - no need to make an additional call to IsUpdating(), because checking for "not FBasicExtentIsValid" is equivalent (and is already there).



- In TListChartSource.SetYValue(), in UpdateExtent(), there is a similar situation.



- In TListChartSource.SetYValue(), there is:

  if FValuesTotalIsValid then
    FValuesTotal += NumberOr(AValue) - NumberOr(oldY);

so, similarly, checking for "FValuesTotalIsValid" is equivalent to making a call to IsUpdating(); since we check "FValuesTotalIsValid", we don't need to call also IsUpdating().



- but the coolest observation is about TListChartSource.UpdateCachesAfterAdd(): in fact, it executes only a useless code when updating.



So I attached your observation to patch_ver2.diff; also similar situation is in TListChartSource.Delete(), so a similar solution is added also there.

wp

2019-04-07 20:00

developer   ~0115307

Done, thank you.

Marcin Wiazowski

2019-04-07 22:50

reporter   ~0115312

Thanks!

Issue History

Date Modified Username Field Change
2019-04-07 02:07 Marcin Wiazowski New Issue
2019-04-07 02:07 Marcin Wiazowski File Added: SpeedTest.zip
2019-04-07 02:07 Marcin Wiazowski File Added: patch.diff
2019-04-07 16:41 wp Assigned To => wp
2019-04-07 16:41 wp Status new => assigned
2019-04-07 17:03 wp Note Added: 0115303
2019-04-07 18:08 Marcin Wiazowski File Added: patch_ver2.diff
2019-04-07 18:09 Marcin Wiazowski Note Added: 0115304
2019-04-07 20:00 wp Fixed in Revision => 60868
2019-04-07 20:00 wp LazTarget => 2.2
2019-04-07 20:00 wp Note Added: 0115307
2019-04-07 20:00 wp Status assigned => resolved
2019-04-07 20:00 wp Resolution open => fixed
2019-04-07 20:00 wp Target Version => 2.2
2019-04-07 22:50 Marcin Wiazowski Note Added: 0115312
2019-04-07 22:50 Marcin Wiazowski Status resolved => closed