mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Add per-row AL performance guidance (#97)
* Add per-row performance guidance Document SetAutoCalcFields for per-row FlowFields and direct writes on iterated records, with focused reviewer retrieval cues. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 85be3fc4-5253-47b8-ba1b-6b8fd188fcea * Bound commit checkpoints by key range Use a capped ordered query to discover each checkpoint watermark before locking and processing only that key range. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 85be3fc4-5253-47b8-ba1b-6b8fd188fcea * Address performance retrieval review Retrieve Commit-in-loop guidance precisely, process exact checkpoint key lists, and narrow clone-before-write discovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 85be3fc4-5253-47b8-ba1b-6b8fd188fcea --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
98af9aa1fc
commit
078b869e33
9 changed files with 174 additions and 18 deletions
|
|
@ -0,0 +1,19 @@
|
|||
codeunit 50493 "Perf Record Clone Bad"
|
||||
{
|
||||
procedure IncreaseCustomerCreditLimits(Percent: Decimal)
|
||||
var
|
||||
Customer: Record Customer;
|
||||
CustomerCopy: Record Customer;
|
||||
begin
|
||||
Customer.SetLoadFields("Credit Limit (LCY)");
|
||||
Customer.SetFilter("Credit Limit (LCY)", '>0');
|
||||
if Customer.FindSet(true) then
|
||||
repeat
|
||||
CustomerCopy.Copy(Customer);
|
||||
CustomerCopy.Validate(
|
||||
"Credit Limit (LCY)",
|
||||
Round(CustomerCopy."Credit Limit (LCY)" * (1 + Percent / 100)));
|
||||
CustomerCopy.Modify(true);
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,17 @@
|
|||
codeunit 50492 "Perf Record Clone Good"
|
||||
{
|
||||
procedure IncreaseCustomerCreditLimits(Percent: Decimal)
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetLoadFields("Credit Limit (LCY)");
|
||||
Customer.SetFilter("Credit Limit (LCY)", '>0');
|
||||
if Customer.FindSet(true) then
|
||||
repeat
|
||||
Customer.Validate(
|
||||
"Credit Limit (LCY)",
|
||||
Round(Customer."Credit Limit (LCY)" * (1 + Percent / 100)));
|
||||
Customer.Modify(true);
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [clone, clone-before-write, copy, gettable, by-value, copied-record, writing-helper]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Avoid cloning records before Modify or Delete in loops
|
||||
|
||||
## Description
|
||||
|
||||
Microsoft's [AL database-method performance guidance](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#insert-modify-delete-and-locktable) states that cloning an iterated record before `Modify` or `Delete` restarts the SQL `SELECT` and issues an extra SQL statement for every row. The runtime treats `Record.Copy`, `RecordRef.GetTable`, and passing a record by value to a writing helper as clones in this situation.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `FindSet(true)` when the loop writes the traversed rows, and call `Modify` or `Delete` on that iterating record variable. If generic code is required, open and iterate the `RecordRef` directly instead of calling `GetTable` for each typed record. Keep a per-row loop when validation or row-specific behavior is required; this rule does not imply that `ModifyAll` or `DeleteAll` is equivalent.
|
||||
|
||||
See sample: `avoid-cloning-records-before-modify-delete-in-loops.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inside an active traversal, copy the current row, convert it with `RecordRef.GetTable`, or pass it without `var` to a helper, then call `Modify` or `Delete` on that clone. Do not flag read-only snapshots, temporary records, or copies used to write a different target table; the documented extra-statement concern is clone-before-write on the traversed table.
|
||||
|
||||
See sample: `avoid-cloning-records-before-modify-delete-in-loops.bad.al`.
|
||||
|
|
@ -1,3 +1,17 @@
|
|||
query 50127 "Perf Customer Chunk"
|
||||
{
|
||||
QueryType = Normal;
|
||||
OrderBy = ascending(CustomerNo);
|
||||
|
||||
elements
|
||||
{
|
||||
dataitem(Customer; Customer)
|
||||
{
|
||||
column(CustomerNo; "No.") { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50128 "Perf Sample CommitInLoop Good"
|
||||
{
|
||||
procedure NormalizeCustomerNames()
|
||||
|
|
@ -5,28 +19,42 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
|
|||
LastCustomerNo: Code[20];
|
||||
begin
|
||||
// The outer loop owns checkpoints; the per-row loop contains no Commit.
|
||||
while NormalizeNextChunk(LastCustomerNo, 500) do
|
||||
while NormalizeNextChunk(LastCustomerNo) do
|
||||
Commit();
|
||||
end;
|
||||
|
||||
local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]; ChunkSize: Integer): Boolean
|
||||
local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean
|
||||
var
|
||||
Customer: Record Customer;
|
||||
RowsInChunk: Integer;
|
||||
TempCustomer: Record Customer temporary;
|
||||
CustomerChunk: Query "Perf Customer Chunk";
|
||||
LastChunkCustomerNo: Code[20];
|
||||
begin
|
||||
Customer.SetCurrentKey("No.");
|
||||
CustomerChunk.TopNumberOfRows(500);
|
||||
if LastCustomerNo <> '' then
|
||||
Customer.SetFilter("No.", '>%1', LastCustomerNo);
|
||||
if not Customer.FindSet(true) then
|
||||
CustomerChunk.SetFilter(CustomerNo, '>%1', LastCustomerNo);
|
||||
CustomerChunk.Open();
|
||||
while CustomerChunk.Read() do begin
|
||||
TempCustomer.Init();
|
||||
TempCustomer."No." := CustomerChunk.CustomerNo;
|
||||
TempCustomer.Insert();
|
||||
LastChunkCustomerNo := CustomerChunk.CustomerNo;
|
||||
end;
|
||||
CustomerChunk.Close();
|
||||
|
||||
if TempCustomer.IsEmpty() then
|
||||
exit(false);
|
||||
|
||||
Customer.LockTable();
|
||||
if TempCustomer.FindSet() then
|
||||
repeat
|
||||
if Customer.Get(TempCustomer."No.") then begin
|
||||
Customer.Name := UpperCase(Customer.Name);
|
||||
Customer.Modify();
|
||||
LastCustomerNo := Customer."No.";
|
||||
RowsInChunk += 1;
|
||||
until (RowsInChunk >= ChunkSize) or (Customer.Next() = 0);
|
||||
end;
|
||||
until TempCustomer.Next() = 0;
|
||||
|
||||
LastCustomerNo := LastChunkCustomerNo;
|
||||
exit(true);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [commit, loop, transaction, lock, checkpoint, codeunit-run]
|
||||
keywords: [commit, commit-in-loop, per-row-commit, checkpoint, bounded-checkpoint, watermark, topnumberofrows]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
|
|
@ -13,16 +13,16 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that each process N rows.
|
||||
Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows.
|
||||
|
||||
## Best Practice
|
||||
|
||||
If the batch is large enough that a single transaction is untenable, use an outer loop that selects and finishes the next N rows. Commit only after the inner row loop has returned and the checkpoint state identifies where the next chunk starts. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
|
||||
If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
|
||||
|
||||
See sample: `avoid-commit-inside-loops.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work.
|
||||
Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
|
||||
|
||||
See sample: `avoid-commit-inside-loops.bad.al`.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 50491 "Perf AutoCalcFields Bad"
|
||||
{
|
||||
procedure CollectOverLimitCustomers(var CustomerNos: List of [Code[20]])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetLoadFields("Credit Limit (LCY)");
|
||||
Customer.SetFilter("Credit Limit (LCY)", '>0');
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
Customer.CalcFields("Balance (LCY)");
|
||||
if Customer."Balance (LCY)" > Customer."Credit Limit (LCY)" then
|
||||
CustomerNos.Add(Customer."No.");
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 50490 "Perf AutoCalcFields Good"
|
||||
{
|
||||
procedure CollectOverLimitCustomers(var CustomerNos: List of [Code[20]])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetLoadFields("Credit Limit (LCY)");
|
||||
Customer.SetFilter("Credit Limit (LCY)", '>0');
|
||||
Customer.SetAutoCalcFields("Balance (LCY)");
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
if Customer."Balance (LCY)" > Customer."Credit Limit (LCY)" then
|
||||
CustomerNos.Add(Customer."No.");
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [setautocalcfields, calcfields, calcsums, flowfield, loop, per-row]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use SetAutoCalcFields when each iterated row needs a FlowField
|
||||
|
||||
## Description
|
||||
|
||||
`Record.SetAutoCalcFields` has been available since runtime 1.0 and makes the specified FlowFields calculate as records are retrieved. Microsoft's [AL database-method performance guidance](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#setautocalcfields) uses it to remove an explicit `CalcFields` call from every iteration when each row's FlowField drives a branch. This is different from `CalcSums`, which returns a total for the filtered set rather than a value for each row.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Call `SetAutoCalcFields` before `FindSet` when every returned row needs the same FlowField for a comparison, branch, or per-record action. Use `CalcSums` instead when the required result is one aggregate over the filtered set (see `calcsums-instead-of-calcfields-in-loop.md`).
|
||||
|
||||
See sample: `use-setautocalcfields-for-per-row-flowfields.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `CalcFields` inside the loop when every iteration reads the same FlowField. Each `CalcFields` request requires a separate SQL statement unless a compatible recent result is cached. Do not replace row-specific decisions with `CalcSums`; an aggregate cannot preserve which rows met the condition.
|
||||
|
||||
See sample: `use-setautocalcfields-for-per-row-flowfields.bad.al`.
|
||||
|
|
@ -38,11 +38,19 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially tables, pages with SourceTable bindings, reports, queries, and codeunits performing record iteration.
|
||||
- The changed procedures and triggers, weighted toward those that perform loops, Find/FindSet/FindFirst calls, CalcFields, CalcSums, FlowField access, or cross-table navigation.
|
||||
- Tokens extracted from the diff that relate to data access and hot-path costs (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `CalcSums`).
|
||||
- The changed procedures and triggers, weighted toward those that perform loops, Find/FindSet/FindFirst calls, CalcFields, SetAutoCalcFields, CalcSums, FlowField access, Commit calls, checkpoint helpers, record copying, RecordRef conversion, Modify/Delete calls, or cross-table navigation.
|
||||
- Tokens extracted from the diff that relate to data access and hot-path costs (`SetRange`, `SetFilter`, `SetLoadFields`, `SetCurrentKey`, `FindSet`, `ReadIsolation`, `LockTable`, `ModifyAll`, `DeleteAll`, `Modify`, `Delete`, `Commit`, `checkpoint`, `Copy`, `RecordRef`, `GetTable`, `TextBuilder`, `Dictionary`, `temporary`, `repeat`, `until`, `CalcFields`, `SetAutoCalcFields`, `CalcSums`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
Apply these targeted cues even when simple token overlap would rank the article below the worklist cutoff:
|
||||
|
||||
- Worklist `use-setautocalcfields-for-per-row-flowfields.md` when a record loop calls `CalcFields`, or when every row reads the same FlowField for a comparison, branch, or per-record action. Worklist `calcsums-instead-of-calcfields-in-loop.md` instead when the loop only accumulates one set total.
|
||||
- Worklist `avoid-commit-inside-loops.md` only when `Commit()` is inside a record-iteration body or a helper invoked once per row. Do not match one `Commit()` after a bounded checkpoint helper returns, a `Commit()` outside iteration, or comments and documentation that merely mention commits.
|
||||
- Worklist `avoid-cloning-records-before-modify-delete-in-loops.md` when an iteration calls `Copy` or `RecordRef.GetTable` before `Modify`/`Delete`, or passes the iterated record without `var` to a helper that writes that record. Do not worklist it from `Modify`, `Delete`, or `RecordRef` alone; exclude a direct write on the iterator, a read-only copy, a temporary record, a different target table, and a `RecordRef` opened and iterated directly.
|
||||
|
||||
These targeted inclusions and exclusions override generic token overlap. Do not retain an excluded article solely because the diff contains one of its keywords.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable performance knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable performance knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue