Care to optimize code?

[duzenko]

I recommend to change from

Code: Select all

function TZGenericCachedResolver.FormInsertStatement(Columns: TObjectList;
  NewRowAccessor: TZRowAccessor): string;
var
  I: Integer;
  Current: TZResolverParameter;
  TableName: string;
  Temp1, Temp2: string;
begin
  TableName := DefineTableName;
  DefineInsertColumns(Columns);
  if Columns.Count = 0 then
  begin
    Result := '';
    Exit;
  end;

  Temp1 := '';
  Temp2 := '';
  for I := 0 to Columns.Count - 1 do
  begin
    Current := TZResolverParameter(Columns[I]);
    if Temp1 <> '' then
      Temp1 := Temp1 + ',';
    Temp1 := Temp1 + IdentifierConvertor.Quote(Current.ColumnName);
    if Temp2 <> '' then
      Temp2 := Temp2 + ',';
    Temp2 := Temp2 + '?';
  end;

  Result := Format('INSERT INTO %s (%s) VALUES (%s)', [TableName, Temp1, Temp2]);
end;

to this code

Code: Select all

function TZGenericCachedResolver.FormInsertStatement(Columns: TObjectList;
  NewRowAccessor: TZRowAccessor): string;
var
  I: Integer;
  Current: TZResolverParameter;
  TableName: string;
  Temp1, Temp2: string;
  l1: Integer;

  procedure Append(const app: String);
  begin
    if Length(Temp1) < l1 + length(app) then
      SetLength(Temp1, 2 * (length(app) + l1));
    Move(app[1], Temp1[l1+1], length(app));
    Inc(l1, length(app));
  end;

begin
  TableName := DefineTableName;
  DefineInsertColumns(Columns);
  if Columns.Count = 0 then
  begin
    Result := '';
    Exit;
  end;

  Temp1 := '';    l1 := 0;
  SetLength(Temp2, 2 * Columns.Count - 1);
  for I := 0 to Columns.Count - 1 do
  begin
    Current := TZResolverParameter(Columns[I]);
    if Temp1 <> '' then
      Append(',');
    Append(IdentifierConvertor.Quote(Current.ColumnName));
    if I > 0 then
      Temp2[I*2] := ',';
    Temp2[I*2+1] := '?';
  end;
  SetLength(Temp1, l1);
  Result := Format('INSERT INTO %s (%s) VALUES (%s)', [TableName, Temp1, Temp2]);
end;

Also, here is my version of TZDefaultIdentifierConvertor.Quote

Code: Select all

function TZDefaultIdentifierConvertor.Quote(const Value: string): string;
var
  QuoteDelim: string;
begin
  if IsCaseSensitive(Value) then
  begin
    QuoteDelim := Metadata.GetIdentifierQuoteString;
    if Length(QuoteDelim) > 1 then
      Result := QuoteDelim[1] + Result + QuoteDelim[2]
    else if Length(QuoteDelim) = 1 then begin
      SetLength(Result, length(Value) + 2);
      Result[1] := QuoteDelim[1];
      Result[length(Result)] := QuoteDelim[1];
      Move(value[1], Result[2], length(value));
      Exit;
    end else
      Result := Value;
  end else
    Result := Value;
end;

[mdaems]

duzenko,

Can you tell me what's better with then new code? At first look it's just more code to do the same job. How dd you get to the new version? by benschmarking it?

Don't misunderstand me. I'm not against a change, it just looks weird to me. Do you have a logical explantion why your version is better/quicker/nicer/...?

Mark

[trupka]

I've been playing a little with TZDefaultIdentifierConvertor.Quote. It's good idea but it seems that it won't always work well

Code: Select all

function TZDefaultIdentifierConvertor.Quote (const Value: string): string;
begin
  ...
      // currently Result is undefined, actually EmptyStr...
      Result := QuoteDelim + Result + QuoteDelim;
     // so, here we actually have Result := QuoteDelim;
  ...
end;

Optimized code could be like:

Code: Select all

function TZDefaultIdentifierConvertor.Quote(const Value: string):string;
var
  QuoteDelim: string;
  QuoteDelimLen, ValueLen: integer;
begin
  if IsCaseSensitive(Value) then
  begin
    QuoteDelim := Metadata.GetIdentifierQuoteString;
    QuoteDelimLen := length(QuoteDelim);
    if QuoteDelimLen > 0 then
	begin
      ValueLen := length(Value);
      SetLength(Result, ValueLen+2);
      Result[1] := QuoteDelim[1];
      Move(Value[1], Result[2], ValueLen);
      if QuoteDelimLen = 1
      then Result[ValueLen+2] := QuoteDelim[1]
      else Result[ValueLen+2] := QuoteDelim[2]
    end
    else Result := Value;
  end
  else Result := Value;
end;

Don't misunderstand me. I'm not against a change, it just looks weird to me. Do you have a logical explantion why your version is better/quicker/nicer/...?

Why is faster?
- profiling says that it's two times faster than original code ( if IsCaseSensitive(Value)), otherwise just a little faster probably because entire code block is smaller (Take a peek at CPU window). Why?
- first line "Result := Value;" is unnecessary if IsCaseSensitive and "costly"
- string contentions are avoided so no System.LStrFromChar and similar calls.
- less memory manager work, SetLength does entire job at once

Why is nicer?
It's not. It's ugly as

Better?
Well, I don't know ... ... faster but harder to read. Mark, I think you should decide what to do with it..

[duzenko]

Well, trupka has explained about TZDefaultIdentifierConvertor.Quote exhaustively and even benchmarked it

Yep, it's ugly, but this is the price for speed. And I did not write it inside "asm .. end", did I?

Regarding TZGenericCachedResolver.FormInsertStatement:
Old code would reallocate memory 4 times for each iteration, while new code will reallocate just a few times for the whole loop.

I did not benchmarked none of functions, but used my bottleneck-finder.

[duzenko]

P.S. I really love easy delphi strings, but sometimes you just have to write C-style code to keep performance high

[mdaems]

Seems the TZDefaultIdentifierConvertor.Quote patch isn't D2009 compatible. I assume the reason is the char array logic.

I can only commit the first patch. That doesn't seem to break the test suite.

SVN Rev 678

Mark

[mdaems]

duzenko,

It seems even the first patch wasn't D2009 safe. Can you have a look at it or should I just rollback?

See post http://zeos.firmos.at/viewtopic.php?t=2482

Mark

[mdaems]

SVN Rev 707 hopefully fixes the D2009 Issue.

Duzenko, do you care for the TZDefaultIdentifierConvertor.Quote patch to be included into a next Zeoslib release? Can you please have a look at the D2009 problem?

Mark