[CLOSED] Improvement of procedure TFPReaderPNG.InternalRead
lazarus / 64-FPC development

----------------------------------------------------------------------
lagprogramming · 2023-06-04 10:20

packages/fcl-image/src/fpreadpng.pp has procedure
TFPReaderPNG.InternalRead (Str:TStream; Img:TFPCustomImage);
The procedure contains the following lines:
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i    
EndOfFile := false;    while not EndOfFile do      begin     
ReadChunk;      HandleChunk;      end;    ZData.position:=0;    
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][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i diff --git
a/packages/fcl-image/src/fpreadpng.pp b/packages/fcl-
image/src/fpreadpng.ppindex 782e7d8556..da7b6a0fa0 100644---
a/packages/fcl-image/src/fpreadpng.pp+++ b/packages/fcl-
image/src/fpreadpng.pp@@ -868,11 +868,10 @@ procedure
TFPReaderPNG.InternalRead (Str:TStream; Img:TFPCustomImage);   ZData
:= TMemoryStream.Create;   try     EndOfFile := false;-    while not
EndOfFile do-      begin+    repeat       ReadChunk;     
 HandleChunk;-      end;+    until EndOfFile;     ZData.position:=0; 
  Decompress := TDecompressionStream.Create (ZData);     try  
Note(reminder):
MSEide/MSEgui has the same procedure in
lib/common/fpccompatibility/msefpreadpng.pas. It uses "FPalette.Free;"
instead of "FreeAndNil(FPalette);".

----------------------------------------------------------------------
wp · 2023-06-04 10:30

If the stream is already at its end the repeat loop will be entered
allowing to read non-existing chunks...

----------------------------------------------------------------------
Martin_fr · 2023-06-04 10:51

Quote from: lagprogramming on June 04, 2023, 12:20:26 pmThe 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.

Quoteand 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 · 2023-06-04 10:53

Quote from: wp on June 04, 2023, 12:30:41 pmIf 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 · 2023-06-04 13:09

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 · 2023-06-04 13:20

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.