mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Sync knowledge articles with review agent instructions
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
f562fba837
commit
5bcdc55df9
62 changed files with 768 additions and 58 deletions
|
|
@ -0,0 +1,20 @@
|
|||
codeunit 50930 "Perf Sample Subscriber Good"
|
||||
{
|
||||
[EventSubscriber(ObjectType::Table, Database::"Sales Line", 'OnAfterValidateEvent', 'No.', false, false)]
|
||||
local procedure OnAfterValidateSalesLineNo(var Rec: Record "Sales Line")
|
||||
var
|
||||
Item: Record Item;
|
||||
begin
|
||||
if Rec.Type <> Rec.Type::Item then
|
||||
exit;
|
||||
|
||||
Item.SetLoadFields("Costing Method");
|
||||
if Item.Get(Rec."No.") then
|
||||
if Item."Costing Method" = Item."Costing Method"::Specific then
|
||||
UpdateSpecificCostingState(Rec);
|
||||
end;
|
||||
|
||||
local procedure UpdateSpecificCostingState(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -17,7 +17,9 @@ Event subscribers run synchronously on the publisher's thread. If a subscriber d
|
|||
|
||||
## Best Practice
|
||||
|
||||
Keep subscribers small: guard early with inexpensive checks, defer heavy work to a task queue or a background session, and cache results across invocations when the data is stable.
|
||||
Keep subscribers small: guard early with inexpensive checks on the publisher record before doing any database work, defer heavy work to a task queue or a background session, and cache results across invocations when the data is stable. In hot events, a cheap `Type`/`Status`/`IsTemporary` exit before a `Get` or `FindFirst` is often the difference between a rare lookup and an N+1 query across every posted line.
|
||||
|
||||
See sample: `keep-event-subscribers-lightweight.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
|
|
|
|||
|
|
@ -2,7 +2,8 @@ codeunit 51204 "Perf Sample LockTable Good"
|
|||
{
|
||||
procedure GetOrCreate(var AgentStatus: Record "Integer"): Boolean
|
||||
begin
|
||||
// Read path: no lock.
|
||||
// Read path: consistent read on this record instance only.
|
||||
AgentStatus.ReadIsolation := IsolationLevel::ReadCommitted;
|
||||
if AgentStatus.Get(1) then
|
||||
exit(true);
|
||||
|
||||
|
|
|
|||
|
|
@ -11,11 +11,11 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
LockTable takes an exclusive write lock on the affected table for the remainder of the transaction. In a helper that is called from many read-only sites and a few write sites, placing LockTable unconditionally at the top serializes every reader on every other reader's lock — the helper becomes a system-wide contention point. The correct shape is a conditional structure: try the read-only path first, and only fall through to LockTable when the code genuinely needs to modify the table.
|
||||
LockTable causes reads against the table to use update locks for the remainder of the transaction. In a helper that is called from many read-only sites and a few write sites, placing LockTable unconditionally at the top serializes every reader on every other reader's lock — the helper becomes a system-wide contention point. The correct shape is a conditional structure: try the read-only path first, and only fall through to LockTable when the code genuinely needs to modify the table.
|
||||
|
||||
## Best Practice
|
||||
|
||||
For paths that are read-only, prefer `ReadIsolation` over `LockTable`. Setting `Rec.ReadIsolation := IsolationLevel::ReadCommitted` on a record variable gives fine-grained, per-instance control over the isolation level without taking an update lock on the table for the rest of the transaction. Use `LockTable` only for paths that genuinely write to the table.
|
||||
For paths that are read-only, prefer `ReadIsolation` over `LockTable`. Setting `Rec.ReadIsolation := IsolationLevel::ReadCommitted` on a record variable gives fine-grained, per-instance control over the isolation level without taking an update lock on the table for the rest of the transaction. Use `ReadCommitted` as the normal read-only choice; move to `RepeatableRead`, `Serializable`, or an update lock only when the code has a concrete consistency invariant that requires it. Use `LockTable` only for paths that genuinely write to the table.
|
||||
|
||||
For helpers that may or may not modify records, factor the code so readers return immediately without a lock and only writers reach the LockTable call. A common pattern: attempt `Rec.Get()` first; if it returns the row, exit with the value; otherwise LockTable and proceed with the Insert. Document the pattern in a comment on the helper so callers understand why the LockTable is inside a branch.
|
||||
|
||||
|
|
|
|||
|
|
@ -11,14 +11,14 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
`ModifyAll` and `DeleteAll` normally compile to a single set-based SQL UPDATE or DELETE. That optimization is conditional: if any subscriber is bound to the table's modify or delete events — `OnBeforeModifyEvent`, `OnAfterModifyEvent`, `OnBeforeDeleteEvent`, `OnAfterDeleteEvent`, and their Rec counterparts — the server must invoke AL per affected row so the subscriber sees each record. The operation falls back to a row-by-row loop, one SQL statement per row, inside the same transaction.
|
||||
`ModifyAll` and `DeleteAll` normally compile to a single set-based SQL UPDATE or DELETE. That optimization is conditional: the server falls back to row-by-row execution when it must invoke AL per affected row. Common causes are global table delete triggers, table modify/delete event subscribers, and Media or MediaSet fields added to the table or a table extension.
|
||||
|
||||
The slowdown is invisible in the caller's source: the call site still reads as a bulk operation. It only shows up under load, and adding an apparently cheap subscriber (even an empty one, or one that guards on a condition and returns) is enough to trigger the fallback for every caller of ModifyAll/DeleteAll on that table across the system. Central tables — Item Ledger Entry, G/L Entry, Sales Line — are the worst places to attach such subscribers because every extension's bulk operation pays the cost.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Before subscribing to a table's modify or delete events, consider whether the logic can live elsewhere — on the triggering action, on a specific OnValidate, or on a business-event publisher. If the subscriber is unavoidable, scope it as narrowly as possible and document that it forces row-by-row execution so future maintainers understand the cost. Watch PRs that add such subscribers to heavily-modified tables.
|
||||
Before subscribing to a table's modify or delete events, consider whether the logic can live elsewhere — on the triggering action, on a specific OnValidate, or on a business-event publisher. If the subscriber, global trigger, or Media/MediaSet field is unavoidable, document that the table may no longer support set-based ModifyAll/DeleteAll. When a table has not regressed, prefer a small number of ModifyAll/DeleteAll calls; they are still commonly 10-50x faster than a manual loop.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An empty or nearly-empty `OnAfterModifyEvent` subscriber on `Sales Line` added as a placeholder for future integration. Every `ModifyAll` on `Sales Line` — in the base app, in every extension, in every tenant — now runs one SQL UPDATE per row.
|
||||
An empty or nearly-empty `OnAfterModifyEvent` subscriber on `Sales Line` added as a placeholder for future integration. Every `ModifyAll` on `Sales Line` — in the base app, in every extension, in every tenant — can now run one SQL UPDATE per row. The same regression can come from a global delete trigger or from adding a Media field to the table.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,16 @@
|
|||
codeunit 50934 "Perf Sample TempLookup Bad"
|
||||
{
|
||||
procedure MarkSeenCustomers(var SalesLine: Record "Sales Line")
|
||||
var
|
||||
TempCustomer: Record Customer temporary;
|
||||
begin
|
||||
if SalesLine.FindSet() then
|
||||
repeat
|
||||
if not TempCustomer.Get(SalesLine."Sell-to Customer No.") then begin
|
||||
TempCustomer.Init();
|
||||
TempCustomer."No." := SalesLine."Sell-to Customer No.";
|
||||
TempCustomer.Insert();
|
||||
end;
|
||||
until SalesLine.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,13 @@
|
|||
codeunit 50933 "Perf Sample Dictionary Good"
|
||||
{
|
||||
procedure MarkSeenCustomers(var SalesLine: Record "Sales Line")
|
||||
var
|
||||
SeenCustomerNos: Dictionary of [Code[20], Boolean];
|
||||
begin
|
||||
if SalesLine.FindSet() then
|
||||
repeat
|
||||
if not SeenCustomerNos.ContainsKey(SalesLine."Sell-to Customer No.") then
|
||||
SeenCustomerNos.Add(SalesLine."Sell-to Customer No.", true);
|
||||
until SalesLine.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [dictionary, temporary-table, lookup, identity, o1]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use Dictionary for temporary identity lookups
|
||||
|
||||
## Description
|
||||
|
||||
A temporary record is useful when code needs record semantics: filters, keys, FlowFields, or table-shaped buffers. When the only operation is "have I seen this key?" or "what value belongs to this key?", a `Dictionary` is the simpler and faster structure. Dictionary lookup is O(1) by key, while a temporary table still pays record and key-management overhead.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `Dictionary` for in-memory lookup sets and maps whose keys fit in memory and whose access pattern is by identity. Keep temporary tables for data that needs table APIs, multiple keys, filter expressions, or later processing as records.
|
||||
|
||||
See sample: `use-dictionary-for-temporary-identity-lookups.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Creating a temporary table solely to call `Get` or `FindFirst` by a single key in a loop. The code looks familiar to AL developers, but it is heavier than the lookup problem requires.
|
||||
|
||||
See sample: `use-dictionary-for-temporary-identity-lookups.bad.al`.
|
||||
|
|
@ -11,11 +11,11 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
FindSet has two modes: FindSet() and FindSet(false) are read-only and take no write lock; FindSet(true) calls LockTable before fetching. Write locks are expensive and hold for the remainder of the transaction, so passing `true` when you do not intend to modify the records increases contention under load.
|
||||
FindSet has two modes: FindSet() and FindSet(false) are read-only and take no update lock; FindSet(true) sets update-lock read isolation on the record before fetching. Update locks are expensive and hold for the lock scope, so passing `true` when you do not intend to modify the records increases contention under load.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Call FindSet with no arguments when the loop only reads field values. Pass `true` only when the same loop is expected to call Modify, Delete, or Rename on the record, and the correctness of the operation depends on the table being locked for the full iteration.
|
||||
Call FindSet with no arguments when the loop only reads field values. Pass `true` only when the same loop is expected to call Modify, Delete, or Rename on the record, and the correctness of the operation depends on the matching rows being locked for the iteration.
|
||||
|
||||
See sample: `use-findset-readonly-by-default.good.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ SetLoadFields instructs the platform to hydrate only the listed fields on a reco
|
|||
|
||||
## Best Practice
|
||||
|
||||
Call SetLoadFields before FindSet, FindFirst, or Get whenever the code path only reads a subset of fields. List every field that is read or written during the operation, including fields used in calculations and downstream function calls. Omitting a field that is later accessed triggers a second round-trip.
|
||||
Call SetLoadFields before FindSet, FindFirst, or Get when the table is wide enough to matter (roughly 10+ fields) and the code path reads a small subset (roughly under 60%) across a material number of rows. Short loops over narrow tables usually do not earn the extra coupling; see `skip-setloadfields-on-narrow-tables-and-short-loops` for that exception. List every field that is read or written during the operation, including fields used in calculations and downstream function calls. Omitting a field that is later accessed triggers a second round-trip.
|
||||
|
||||
Fields that appear **only** in SetRange or SetFilter calls do not need to be included — the database resolves the filter using the index without hydrating the value into AL memory. Including filter-only fields wastes bandwidth and is not required.
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 50932 "Perf Sample TextConcat Bad"
|
||||
{
|
||||
procedure BuildItemList(var Item: Record Item): Text
|
||||
var
|
||||
Result: Text;
|
||||
begin
|
||||
if Item.FindSet() then
|
||||
repeat
|
||||
Result += StrSubstNo('%1,%2', Item."No.", Item.Description);
|
||||
until Item.Next() = 0;
|
||||
|
||||
exit(Result);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
codeunit 50931 "Perf Sample TextBuilder Good"
|
||||
{
|
||||
procedure BuildItemList(var Item: Record Item): Text
|
||||
var
|
||||
Builder: TextBuilder;
|
||||
begin
|
||||
if Item.FindSet() then
|
||||
repeat
|
||||
Builder.AppendLine(StrSubstNo('%1,%2', Item."No.", Item.Description));
|
||||
until Item.Next() = 0;
|
||||
|
||||
exit(Builder.ToText());
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [textbuilder, string-concatenation, loop, text, allocation]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use TextBuilder for loop-based string assembly
|
||||
|
||||
## Description
|
||||
|
||||
Repeated `Text := Text + ...` concatenation inside a loop reallocates and copies the growing string on every iteration. In AL, `TextBuilder` is the platform type for constructing larger text payloads incrementally. `StrSubstNo` remains appropriate for formatting one message; TextBuilder is for many appends, especially inside loops.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use `TextBuilder.Append` or `AppendLine` when assembling CSV rows, log payloads, JSON-ish diagnostic text, or other multi-line strings from repeated loop iterations. Convert to Text once, after the loop, with `ToText()`.
|
||||
|
||||
See sample: `use-textbuilder-for-loop-string-assembly.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Appending to the same Text variable on every iteration of a large loop. Each append copies the accumulated prefix again, so the cost grows with both row count and final string length.
|
||||
|
||||
See sample: `use-textbuilder-for-loop-string-assembly.bad.al`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue