TMS scripter slow AutoComplete. +fixfor Wagner

BIZ TMS Scripter
1

Hi!


AutoComplete for empty string '' (I mean, when I precc Ctrl+Space in memo) works noticebly slow (everything pauses on several seconds), especially when editing unit with a form.
There are 2 cases to see how slow it is:
1) Press Ctrl+Space on a new line (wait for... wait... wait... wait few more seconds) finally see AutoComplete box
2) Type: "M" and then press Ctrl+Space (autocomplete box will appear relatively faster, displaying all the items that start with M). Now press Backspace to delete "M" and prepare to wait. This one is especially annoying as it happens while typing and when you don't expect for everything to freeze.

I traced into autocomplete code and found out that most of the time is eaten by TScripterCodeInsight.FillAllMatchingItems routine.
(FScript.Scripter.DefaultInstances.Count in my case contains about 50 units) 

As I see in TMS code there's a plenty of room to optimization. 
For example TScripterCodeInsight.FillMatchingItems do a lot of repeating job, and can be optimized a little bit. 
1) there's a lot of calls to UpperCase(AName) in loops, which should be performed only once
2) all the properties and methods are accessed by index at least 3 times in a loop (this number can be reduced to 1) (and, yes, compiler is not that clever to do that job for you)
These trivial optimizations can give a huge boost in performance.

Original:
procedure TScripterCodeInsight.FillMatchingItems(AClass: TatClass; AName: string; AList: TStrings);
var
  c: integer;
begin
  if AClass = nil then
    exit;

  { try go into the class based on the name passed }
  with AClass do
  begin
    { Since it's final node, search for a partial name }
    for c := 0 to AClass.Properties.Count - 1 do
      if Copy(UpperCase(AClass.Properties[c].Name), 1, Length(AName)) = UpperCase(AName) then
        AddItemToList(AList, AClass.FName, AClass.Properties[c].Name, AClass.Properties[c]);

    for c := 0 to AClass.Methods.Count - 1 do
      if Copy(UpperCase(AClass.Methods[c].Name), 1, Length(AName)) = UpperCase(AName) then
        AddItemToList(AList, AClass.FName, AClass.Methods[c].Name, AClass.Methods[c]);
  end;
end;

Optimized v1:
 procedure TScripterCodeInsight.FillMatchingItems(AClass: TatClass; AName: string; AList: TStrings);
var
  c: integer;
  tmpUpperName: string;
  tmpNameLen: Integer;
  tmpProp: TatProperty;
  tmpMeth: TatMethod;
begin
  if AClass = nil then
    exit;

  tmpUpperName := UpperCase(AName);
  tmpNameLen := Length(AName);

  { try go into the class based on the name passed }
  with AClass do
  begin
    { Since it's final node, search for a partial name }
    for c := 0 to AClass.Properties.Count - 1 do
    begin
      tmpProp := AClass.Properties[c];
      if Copy(UpperCase(tmpProp.Name), 1, tmpNameLen) = tmpUpperName then
        AddItemToList(AList, AClass.FName, tmpProp.Name, tmpProp);
    end;

    for c := 0 to AClass.Methods.Count - 1 do
    begin
      tmpMeth := AClass.Methods[c];
      if Copy(UpperCase(tmpMeth.Name), 1, tmpNameLen) = tmpUpperName then
        AddItemToList(AList, AClass.FName, tmpMeth.Name, tmpMeth);
    end;
  end;
end;



I'm using v6.5.2
2
Additionally, in the case when Length(AName) = 0 (as in my case when I run Autocomplete for empty text). there's no need at all to calculate the Copy(UpperCase(tmpMeth.Name), 1, tmpNameLen) = tmpUpperName.

So there can be done more optimizations:

if tmpNameLen = 0 then
begin
    for c := 0 to AClass.Properties.Count - 1 do
    begin
      tmpProp := AClass.Properties[c];
      AddItemToList(AList, AClass.FName, tmpProp.Name, tmpProp);
    end;

    for c := 0 to AClass.Methods.Count - 1 do
    begin
      tmpMeth := AClass.Methods[c];
      AddItemToList(AList, AClass.FName, tmpMeth.Name, tmpMeth);
    end;
end
else
begin
    for c := 0 to AClass.Properties.Count - 1 do
    begin
      tmpProp := AClass.Properties[c];
      if Copy(UpperCase(tmpProp.Name), 1, tmpNameLen) = tmpUpperName then
        AddItemToList(AList, AClass.FName, tmpProp.Name, tmpProp);
    end;

    for c := 0 to AClass.Methods.Count - 1 do
    begin
      tmpMeth := AClass.Methods[c];
      if Copy(UpperCase(tmpMeth.Name), 1, tmpNameLen) = tmpUpperName then
        AddItemToList(AList, AClass.FName, tmpMeth.Name, tmpMeth);
    end;
end;
3
And here is on more optimization tip. This time it gives the really noticeable boost in performance (if Complete boolean evaluation compiler option is not enabled. Otherwise, performance will be just the same).

At the routine TScripterCodeInsight.AddItemToList, replace the line:
    if (AnsiCompareText(AList, AName) = 0) and (AList.Objects = AValue) then
with
    if (AList.Objects = AValue) and (AnsiCompareText(AList, AName) = 0) then

As 2 pointers comparison will execute much faster than a call to AnsiCompareText. 
4

Hi Alexksandrs,

thanks for the feedback. We have implemented the changes, and the last one did increase performance indeed. We didn't notice a bad performance before because it's unusual (for us) to have that many DefaultInstances added. We forced it here to have many and then we could optimize.
Next release will include those changes.
5

Thank you, Wagner. I'm really glad, to hear that. =)


Please, consider the possibility to optimize the other code (if it exist, and if you have time for that) in a similar manner. (introducing "explaning" local variable to get rid of multiple "find item by index").
6

What other code? Not sure what you mean by local variables?

7

Sorry for expressing my thought no clear enough.

I mean that replacing multiple uses of SomeList[aIndex] with a local variable (assigned with a single call to SomeList[aIndex]) will give more efficient (faster) code. (See my post "Optiimized v1")

8

I'm not sure if it makes that much difference. Your last suggestion was what made the difference here, because it avoided several loops. If you have any specific situation you are experiencing performance issues, just let me know.

9
> I'm not sure if it makes that much difference.
Sorry, I don't have aqTime currently installed to provide the benchmarks data. 
But, the difference is not that big as in the case with last suggestion, although it still makes the code faster. Delphi compiler is not that smart/stupid to automatically replace multiple calls to indexed property with a single one. So if you do access properties by index several times in the same method, you can be sure, that compiler will search that property by it's index each and every time. 
And the methods I optimized (v1 & v2) are called several hundrends times EACH time AutoComplete is invoked (Ctrl+Space is pressed).

Additionally, these optimizations make the code less error-prone.

So it's up to you, whether you'll wait for another bug report (which might/or might not appear later), or make the code faster right now.
10

Of course I changed all parts of the source code, the code is better anyway.

I'm just mentioning that the focus is the third, last change, that was the one which really improved it.