Recent

Author Topic: [CLOSED] Improvement of procedure TFPReaderPNG.InternalRead  (Read 960 times)

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
packages/fcl-image/src/fpreadpng.pp has procedure TFPReaderPNG.InternalRead (Str:TStream; Img:TFPCustomImage);
The procedure contains the following lines:
Code: Pascal  [Select][+][-]
  1.     EndOfFile := false;
  2.     while not EndOfFile do
  3.       begin
  4.       ReadChunk;
  5.       HandleChunk;
  6.       end;
  7.     ZData.position:=0;
  8.  
   
The patch replaces the "while" loop with a "repeat" one. The number of pascal lines is reduced and the compiler will not generate a useless unconditional jump.

Code: Pascal  [Select][+][-]
  1. diff --git a/packages/fcl-image/src/fpreadpng.pp b/packages/fcl-image/src/fpreadpng.pp
  2. index 782e7d8556..da7b6a0fa0 100644
  3. --- a/packages/fcl-image/src/fpreadpng.pp
  4. +++ b/packages/fcl-image/src/fpreadpng.pp
  5. @@ -868,11 +868,10 @@ procedure TFPReaderPNG.InternalRead (Str:TStream; Img:TFPCustomImage);
  6.    ZData := TMemoryStream.Create;
  7.    try
  8.      EndOfFile := false;
  9. -    while not EndOfFile do
  10. -      begin
  11. +    repeat
  12.        ReadChunk;
  13.        HandleChunk;
  14. -      end;
  15. +    until EndOfFile;
  16.      ZData.position:=0;
  17.      Decompress := TDecompressionStream.Create (ZData);
  18.      try
   
Note(reminder):
MSEide/MSEgui has the same procedure in lib/common/fpccompatibility/msefpreadpng.pas. It uses "FPalette.Free;" instead of "FreeAndNil(FPalette);".
« Last Edit: June 13, 2023, 10:45:35 am by lagprogramming »

wp

  • Hero Member
  • *****
  • Posts: 11855
Re: Improvement of procedure TFPReaderPNG.InternalRead
« Reply #1 on: June 04, 2023, 12:30:41 pm »
If the stream is already at its end the repeat loop will be entered allowing to read non-existing chunks...

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9792
  • Debugger - SynEdit - and more
    • wiki
Re: Improvement of procedure TFPReaderPNG.InternalRead
« Reply #2 on: June 04, 2023, 12:51:37 pm »
The patch replaces the "while" loop with a "repeat" one. The number of pascal lines is reduced
Is not in itself an optimization. It would have to be an (somehow objectively measurable) improvement of readability.

Quote
and the compiler will not generate a useless unconditional jump.

Such "do the compilers work in user code" optimizations should (with maybe incredible few exceptions) be avoided.
The next version of the compiler may just do the reverse, and produce worse code with the change.


Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9792
  • Debugger - SynEdit - and more
    • wiki
Re: Improvement of procedure TFPReaderPNG.InternalRead
« Reply #3 on: June 04, 2023, 12:53:25 pm »
If the stream is already at its end the repeat loop will be entered allowing to read non-existing chunks...

Not if the variable is set one line above...

lagprogramming

  • Sr. Member
  • ****
  • Posts: 405
Re: Improvement of procedure TFPReaderPNG.InternalRead
« Reply #4 on: June 04, 2023, 03:09:17 pm »
There is a reason why pascal has both while and repeat loops, it avoids a useless initial check.
Regarding compiler optimizations, I think that the compiler should be able to optimize well written pascal code with priority against poorly written code. A poorly written pascal code can be manually optimized up to the level of being a well written pascal code.
In this example, EndOfFile is a boolean variable. Setting it to false and immediately after checking it's value is not the best programming practice. Most likely, the official code made wp think EndOfFile is a property.

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9792
  • Debugger - SynEdit - and more
    • wiki
Re: Improvement of procedure TFPReaderPNG.InternalRead
« Reply #5 on: June 04, 2023, 03:20:51 pm »
Not generally against this change. Just the verbally expressed arguments...

- Saving a line of source code is not an optimization on its own. (besides that can be done by deleting the linebreak).
- Leading to less assembler => even less a reason. That is why we don't code in asm.

+ Making it clear, that the condition is always wrong before the first iteration of the loop => IMHO that is a reason.



"less assembler"
In some bottle neck cases, this may be needed, until the compiler can do it.

Though, then it should be "faster asm" as the reason, which is very hard to establish given the amount of targets there are.
"less assembler" may sometimes be slower.

In this case, it will likely not be slower. If it will be faster is a different question.



 

TinyPortal © 2005-2018