gabr42 / OmniThreadLibrary

A simple and powerful multithreading library for Delphi
http://www.omnithreadlibrary.com
Other
466 stars 142 forks source link

parallel.foreach Memory Leak #200

Closed Uefi1 closed 1 week ago

Uefi1 commented 1 week ago

Hi I am facing two problems:

var
List:Tstringlist;
begin
List:=TStringList.Create;
Parallel.ForEach<string>(List).Execute(
procedure (const value: string)
var
Line:string;
begin
Line:=value;
end);
end;

Huge memory leak

and like this

var
List:Tstringlist;
begin
List:=TStringList.Create;
Parallel.ForEach<integer>(List).Execute(
procedure (const value: integer)
var
Line:string;
begin
Line:=List[value];
end);
end;

Error:

First chance exception at $000007FEFD9CBE0D. Exception class EOmniValueConv with message 'Value 26960560128 is too big to fit into Integer'.

TommiPrami commented 1 week ago

Not sure if it is the same, but in my hobby project started to show memory leaks with latest version of FastMM5, which had some bug fix to the leak reporting...

I think it uses Parallel.ForEach tough.

gabr42 commented 1 week ago

This code only leaks a TStringList. Please submit a minimal reproducible example.

https://stackoverflow.com/help/minimal-reproducible-example

Uefi1 commented 1 week ago

I realized something what could be the problem Parallel.ForEach(List).Execute does work asynchronously why ?

gabr42 commented 1 week ago

ForEach should work asynchronously only if you use the .NoWait modifier.

And use Parallel.For, it is much faster.

And read the book :)

http://www.omnithreadlibrary.com/documentation/#book

Uefi1 commented 1 week ago

ForEach should work asynchronously only if you use the .NoWait modifier.

And use Parallel.For, it is much faster.

And read the book :)

http://www.omnithreadlibrary.com/documentation/#book

Hello, first of all, thank you for your advice and desire to help, please study this example, this is where memory leaks occur:

unit Unit2;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls, OtlParallel;

type
  TForm2 = class(TForm)
    Button1: TButton;
    Memo1: TMemo;
    OpenDialog1: TOpenDialog;
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form2: TForm2;

implementation

{$R *.dfm}

procedure ReadF(const fname:string);
const
BUFSIZE=1024*18;
var
List:TStringList;
Reader: TStreamReader;
Buf: array [0..BUFSIZE] of char;
begin
List:=TStringList.Create;
Reader := TStreamReader.Create(fname, TEncoding.Default, True, 4096);
while not Reader.EndOfStream do begin
TThread.Synchronize(nil,
procedure
begin
Reader.ReadBlock(@Buf, 0, BUFSIZE);
end);
List.Text:=Buf;
parallel.ForEach(0, List.Count-1).Execute(
procedure (const value:integer)
var
Line:string;
begin
Line:=List[value];
end);
end;
List.Free;
Reader.Close;
FreeAndNil(Reader);
end;

procedure TForm2.Button1Click(Sender: TObject);
begin
if opendialog1.Execute then
ReadF(opendialog1.filename);
end;

end.
gabr42 commented 1 week ago

Which Delphi is that? Because the code doesn't compile with 12.2.

Also, why are you reading data with Synchronize? This is running in a main thread, no nead for synchronize.

gabr42 commented 1 week ago

I did modify your code like this and I see no leaks:

procedure ReadF(const fname: string);
const
  BUFSIZE = 1024 * 18;
var
  List: TStringList;
  Reader: TStreamReader;
  Buf: TCharArray;
begin
  SetLength(Buf, BUFSIZE + 1);
  List := TStringList.Create;
  Reader := TStreamReader.Create(fname, TEncoding.Default, True, 4096);
  while not Reader.EndOfStream do
  begin
    TThread.Synchronize(nil,
      procedure
      begin
        Reader.ReadBlock(Buf, 0, BUFSIZE);
      end);
    List.Text := string(Buf);
    parallel.ForEach(0, List.Count - 1).Execute(
      procedure(const value: integer)
      var
        Line: string;
      begin
        Line := List[value];
      end);
  end;
  List.Free;
  Reader.Close;
  FreeAndNil(Reader);
end;
gabr42 commented 1 week ago

I'm running this with FastMM4 from https://github.com/pleriche/FastMM4 in FullDebugMode.

Uefi1 commented 1 week ago

I'm running this with FastMM4 from https://github.com/pleriche/FastMM4 in FullDebugMode.

Hello again, leaks are observed and with your provided code, do not trust FastMM, open the task manager and look =) My Delphi its Delphi XE2

TommiPrami commented 1 week ago

I'm running this with FastMM4 from https://github.com/pleriche/FastMM4 in FullDebugMode.

Hello again, leaks are observed and with your provided code, do not trust FastMM, open the task manager and look =) My Delphi its Delphi XE2

With the task manager you just observe the memory manager and so on.

I would trust FastMM report way more.

And if you see live leak, that will just allocate memory indefinetöy, as long as the paralle code is running. That is a different story, but in this case you should provide some kind of proof.

-tee-

gabr42 commented 1 week ago

So, you are not talking about memory leaks but about the fact that memory is used and not released until Button1Click exits.

That is normal. You are not processing Windows messages while this code is running. OTL does the cleanup via Windows messages.

I would use a different set of parallel patterns to solve your problem. You could create a 'Pipeline' with one stage, running in N copies (as much as you have CPUs - 1). Then you can read the data in the main thread and write blocks to be processed into the pipeline. Inside the pipeline, each copy of the stage will grab its packet of data, process it and spit the result out.

You would have to fix your code though because it just breaks input data in the middle of the line (where the buffer ends), but that is a different problem (and occurs with the current code, too).

Learn more about pipelines here: http://www.omnithreadlibrary.com/book/chap06.html#highlevel-pipeline

Uefi1 commented 1 week ago

So, you are not talking about memory leaks but about the fact that memory is used and not released until Button1Click exits.

That is normal. You are not processing Windows messages while this code is running. OTL does the cleanup via Windows messages.

I would use a different set of parallel patterns to solve your problem. You could create a 'Pipeline' with one stage, running in N copies (as much as you have CPUs - 1). Then you can read the data in the main thread and write blocks to be processed into the pipeline. Inside the pipeline, each copy of the stage will grab its packet of data, process it and spit the result out.

You would have to fix your code though because it just breaks input data in the middle of the line (where the buffer ends), but that is a different problem (and occurs with the current code, too).

Learn more about pipelines here: http://www.omnithreadlibrary.com/book/chap06.html#highlevel-pipeline

No, this is not normal on a 100 megabyte file, it eats 10 gigabytes of RAM, it’s a leak!

gabr42 commented 1 week ago

Works as designed.