[CLOSED] Improvements in rtl/objpas/sysutils/fina.inc

[lagprogramming]

rtl/objpas/sysutils/fina.inc has the folowing routines:
function ChangeFileExt(const FileName, Extension: PathStr): PathStr;
function ExtractFilePath(const FileName: PathStr): PathStr;
function ExtractFileDir(const FileName: PathStr): PathStr;
function ExtractFileName(const FileName: PathStr): PathStr;
function ExtractFileExt(const FileName:PathStr):PathStr;

Switching the "I" and "EndSep" assignment lines makes the compiler produce better code. You can verify by looking at the generated assembly file.
I'm talking about the lines like:

Code: Pascal  [Select][+][-]

1.   I := Length(FileName);
2.   EndSep:=AllowDirectorySeparators+AllowDriveSeparators;

 
The following patch modifies all the above functions.

Code: Pascal  [Select][+][-]

1. diff --git a/rtl/objpas/sysutils/fina.inc b/rtl/objpas/sysutils/fina.inc
2. index e817d60e6a..95217a026b 100644
3. --- a/rtl/objpas/sysutils/fina.inc
4. +++ b/rtl/objpas/sysutils/fina.inc
5. @@ -21,8 +21,8 @@ function ChangeFileExt(const FileName, Extension: PathStr): PathStr;
6.    SOF : Boolean;
7.    
8.  begin
9. -  i := Length(FileName);
10.    EndSep:=AllowDirectorySeparators+AllowDriveSeparators+[ExtensionSeparator];
11. +  i := Length(FileName);
12.    while (I > 0) and not(FileName[I] in EndSep) do
13.      Dec(I);
14.    if (I = 0) or (FileName[I] <> ExtensionSeparator) then
15. @@ -41,8 +41,8 @@ function ExtractFilePath(const FileName: PathStr): PathStr;
16.    i : longint;
17.    EndSep : Set of Char;
18.  begin
19. -  i := Length(FileName);
20.    EndSep:=AllowDirectorySeparators+AllowDriveSeparators;
21. +  i := Length(FileName);
22.    while (i > 0) and not CharInSet(FileName[i],EndSep) do
23.      Dec(i);
24.    If I>0 then
25. @@ -56,8 +56,8 @@ function ExtractFileDir(const FileName: PathStr): PathStr;
26.    i : longint;
27.    EndSep : Set of Char;
28.  begin
29. -  I := Length(FileName);
30.    EndSep:=AllowDirectorySeparators+AllowDriveSeparators;
31. +  I := Length(FileName);
32.    while (I > 0) and not CharInSet(FileName[I],EndSep) do
33.      Dec(I);
34.    if (I > 1) and CharInSet(FileName[I],AllowDirectorySeparators) and
35. @@ -102,8 +102,8 @@ function ExtractFileName(const FileName: PathStr): PathStr;
36.    i : longint;
37.    EndSep : Set of Char;
38.  begin
39. -  I := Length(FileName);
40.    EndSep:=AllowDirectorySeparators+AllowDriveSeparators;
41. +  I := Length(FileName);
42.    while (I > 0) and not CharInSet(FileName[I],EndSep) do
43.      Dec(I);
44.    Result := Copy(FileName, I + 1, MaxInt);
45. @@ -117,8 +117,8 @@ function ExtractFileExt(const FileName: PathStr): PathStr;
46.    
47.  begin
48.    Result:='';
49. -  I := Length(FileName);
50.    EndSep:=AllowDirectorySeparators+AllowDriveSeparators+[ExtensionSeparator];
51. +  I := Length(FileName);
52.    while (I > 0) and not CharInSet(FileName[I],EndSep) do
53.      Dec(I);
54.    if (I > 0) and (FileName[I] = ExtensionSeparator) then

[Thaddy]

Why would you slow down the code with this patch?

[marcov]

The window that the variable is live shortens, increasing the chances that it can be registered. Perhaps the compiler should be able to do this automatically though :-)

[lagprogramming]

AllowDirectorySeparators, AllowDriveSeparators and ExtensionSeparator are all constants, but I couldn't turn EndSep from a variable to a constant. Turning EndSep variables into constants will make the patch obsolete.
I mean I've tried adding

Code: Pascal  [Select][+][-]

1. const
2.   EndSep : Set of Char = AllowDirectorySeparators+AllowDriveSeparators+[ExtensionSeparator];

in order to help the compiler produce better code but the compiler refused to build the modified file.
It might be something trivial to make the compiler compute the EndSep values at compile time, I don't know, or maybe it should already compute the EndSep values at compile time and there is a bug. 

[PascalDragon]

AllowDirectorySeparators, AllowDriveSeparators and ExtensionSeparator are all constants, but I couldn't turn EndSep from a variable to a constant. Turning EndSep variables into constants will make the patch obsolete.
I mean I've tried adding

Code: Pascal  [Select][+][-]

1. const
2.   EndSep : Set of Char = AllowDirectorySeparators+AllowDriveSeparators+[ExtensionSeparator];

in order to help the compiler produce better code but the compiler refused to build the modified file.
It might be something trivial to make the compiler compute the EndSep values at compile time, I don't know, or maybe it should already compute the EndSep values at compile time and there is a bug. 

AllowDirectorySeperators and AllowDriveSeparators are typed constants. Typed constants can not be used as an initializer for constants (typed or untyped) or variables, thus you simply can't declare a EndSep constant.

[lagprogramming]

AllowDirectorySeperators and AllowDriveSeparators are typed constants. Typed constants can not be used as an initializer for constants (typed or untyped) or variables, thus you simply can't declare a EndSep constant.

Trying to use conditional compilation fails too

Code: Pascal  [Select][+][-]

1. {$if AllowDirectorySeparators=['\','/']}
2. //Code
3. {$endif}

So, either we wait for the compiler to be modified in order to be able to compute at compile time these constant sets, either the local EndSep variables would be replaced by global unit variables that would be computed at unit initialization(this should work, right?). I have doubts the last one would be accepted in the official sources.
If you have a long list of files that you should sort using such routines, it will take a lot of time.
I don't know which solution should be applied because I have no idea how much work has to be done in order to make the compiler optimize such code. Computing the values at compile time would be my first choice.

[PascalDragon]

AllowDirectorySeperators and AllowDriveSeparators are typed constants. Typed constants can not be used as an initializer for constants (typed or untyped) or variables, thus you simply can't declare a EndSep constant.

Trying to use conditional compilation fails too

Code: Pascal  [Select][+][-]

1. {$if AllowDirectorySeparators=['\','/']}
2. //Code
3. {$endif}

Same reason. Typed constants can not be used in compiler directives.

So, either we wait for the compiler to be modified in order to be able to compute at compile time these constant sets,

In principle one could already declare these constants as untyped set constants (I don't know right whether there is a reason that would prohibit changing this as typed and untyped constants behave different in some circumstances - e.g. you can't take the address of an untyped constant):

Code: Pascal  [Select][+][-]

1. const
2.   AllowDirectorySeparators = ['/', '\'];

But the compiler won't get the ability to calculate with typed constants at compile time.

either the local EndSep variables would be replaced by global unit variables that would be computed at unit initialization(this should work, right?). I have doubts the last one would be accepted in the official sources.

Well, you could use a local writable constant (which is essentially a static variable) - though I don't say that we'd accept that:

Code: Pascal  [Select][+][-]

1. procedure SomeProc;
2. {$push}{$J+}
3. const
4.   EndSep: set of Char = [];
5. {$pop}
6. begin
7.   if EndSep = [] then
8.     EndSep := AllowDirectorySeparators + AllowDriveSeparators;
9.   {... }
10. end;

From the second call of SomeProc on the value of EndSep will no longer be empty. This won't be threadsafe however (it could be fixed by using something like InterlockedExchange however).

[lagprogramming]

Why would you slow down the code with this patch?

When the CPU target was x86_64, for each modified routine I've noticed one less push/pop pair instructions.