From 6a75a425af34b0ccc6bac808674087c08072de03 Mon Sep 17 00:00:00 2001 From: Stefano Demiliani Date: Mon, 24 Aug 2026 16:06:07 +0200 Subject: [PATCH] fix(community/performance): address review feedback --- .../changecompany-in-loop-drops-caches.bad.al | 1 + ...changecompany-in-loop-drops-caches.good.al | 20 +++----- .../changecompany-in-loop-drops-caches.md | 2 +- .../countapprox-for-progress-not-count.bad.al | 22 -------- ...countapprox-for-progress-not-count.good.al | 21 -------- .../countapprox-for-progress-not-count.md | 28 ---------- ...llowed-guard-on-pages-used-as-odata.bad.al | 14 ++++- ...lowed-guard-on-pages-used-as-odata.good.al | 12 ++++- ...side-write-transaction-holds-locks.good.al | 51 ++++++++++++++++--- ...nt-inside-write-transaction-holds-locks.md | 4 +- ...false-does-not-skip-page-field-cost.bad.al | 23 --------- ...alse-does-not-skip-page-field-cost.good.al | 18 ------- ...ble-false-does-not-skip-page-field-cost.md | 28 ---------- 13 files changed, 79 insertions(+), 165 deletions(-) delete mode 100644 community/knowledge/performance/countapprox-for-progress-not-count.bad.al delete mode 100644 community/knowledge/performance/countapprox-for-progress-not-count.good.al delete mode 100644 community/knowledge/performance/countapprox-for-progress-not-count.md delete mode 100644 community/knowledge/performance/visible-false-does-not-skip-page-field-cost.bad.al delete mode 100644 community/knowledge/performance/visible-false-does-not-skip-page-field-cost.good.al delete mode 100644 community/knowledge/performance/visible-false-does-not-skip-page-field-cost.md diff --git a/community/knowledge/performance/changecompany-in-loop-drops-caches.bad.al b/community/knowledge/performance/changecompany-in-loop-drops-caches.bad.al index f1246a6..12aefe0 100644 --- a/community/knowledge/performance/changecompany-in-loop-drops-caches.bad.al +++ b/community/knowledge/performance/changecompany-in-loop-drops-caches.bad.al @@ -5,6 +5,7 @@ codeunit 50100 "ChangeCompany Loop Bad" Customer: Record Customer; Company: Record Company; begin + Customer.SetLoadFields(Name); if Buffer.FindSet() then repeat if Company.FindSet() then diff --git a/community/knowledge/performance/changecompany-in-loop-drops-caches.good.al b/community/knowledge/performance/changecompany-in-loop-drops-caches.good.al index e26a98f..ffb4f3b 100644 --- a/community/knowledge/performance/changecompany-in-loop-drops-caches.good.al +++ b/community/knowledge/performance/changecompany-in-loop-drops-caches.good.al @@ -1,25 +1,19 @@ codeunit 50100 "ChangeCompany Loop Good" { - procedure NameInCompany(CompanyNameValue: Text[30]; CustomerNo: Code[20]): Text + procedure NamesForCustomers(var Buffer: Record Customer) var Customer: Record Customer; + Company: Record Company; begin - Customer.ChangeCompany(CompanyNameValue); Customer.SetLoadFields(Name); - if Customer.Get(CustomerNo) then - exit(Customer.Name); - end; - - procedure NamesForCompanies(var Company: Record Company) - var - Customer: Record Customer; - begin if Company.FindSet() then repeat Customer.ChangeCompany(Company.Name); - Customer.SetLoadFields(Name); - if Customer.FindFirst() then - Message(Customer.Name); + if Buffer.FindSet() then + repeat + if Customer.Get(Buffer."No.") then + Message(Customer.Name); + until Buffer.Next() = 0; until Company.Next() = 0; end; } diff --git a/community/knowledge/performance/changecompany-in-loop-drops-caches.md b/community/knowledge/performance/changecompany-in-loop-drops-caches.md index 816c0a5..fddef76 100644 --- a/community/knowledge/performance/changecompany-in-loop-drops-caches.md +++ b/community/knowledge/performance/changecompany-in-loop-drops-caches.md @@ -17,7 +17,7 @@ application-area: [all] ## Best Practice -Group work by company. Call `ChangeCompany` once per distinct company, then `FindSet`/`Get` that company's rows. Reset the variable back when the batch finishes. +Group work by company. Call `ChangeCompany` once per distinct company, then `FindSet`/`Get` that company's rows. If the record variable is reused afterward, call `ChangeCompany()` without a company name to redirect it back to the current company. See sample: `changecompany-in-loop-drops-caches.good.al`. diff --git a/community/knowledge/performance/countapprox-for-progress-not-count.bad.al b/community/knowledge/performance/countapprox-for-progress-not-count.bad.al deleted file mode 100644 index 46dee20..0000000 --- a/community/knowledge/performance/countapprox-for-progress-not-count.bad.al +++ /dev/null @@ -1,22 +0,0 @@ -codeunit 50100 "CountApprox Progress Bad" -{ - procedure RecalcUsCustomers() - var - Customer: Record Customer; - Window: Dialog; - Counter: Integer; - Total: Integer; - begin - Customer.SetRange("Country/Region Code", 'US'); - // Exact Count() is a SELECT COUNT(*) just to drive a progress bar. - Total := Customer.Count(); - Window.Open('Processing #1###### of #2######'); - if Customer.FindSet() then - repeat - Counter += 1; - Window.Update(1, Counter); - Window.Update(2, Total); - until Customer.Next() = 0; - Window.Close(); - end; -} diff --git a/community/knowledge/performance/countapprox-for-progress-not-count.good.al b/community/knowledge/performance/countapprox-for-progress-not-count.good.al deleted file mode 100644 index ed2f22d..0000000 --- a/community/knowledge/performance/countapprox-for-progress-not-count.good.al +++ /dev/null @@ -1,21 +0,0 @@ -codeunit 50100 "CountApprox Progress Good" -{ - procedure RecalcUsCustomers() - var - Customer: Record Customer; - Window: Dialog; - Counter: Integer; - Total: Integer; - begin - Customer.SetRange("Country/Region Code", 'US'); - Total := Customer.CountApprox(); - Window.Open('Processing #1###### of #2######'); - if Customer.FindSet() then - repeat - Counter += 1; - Window.Update(1, Counter); - Window.Update(2, Total); - until Customer.Next() = 0; - Window.Close(); - end; -} diff --git a/community/knowledge/performance/countapprox-for-progress-not-count.md b/community/knowledge/performance/countapprox-for-progress-not-count.md deleted file mode 100644 index 4261658..0000000 --- a/community/knowledge/performance/countapprox-for-progress-not-count.md +++ /dev/null @@ -1,28 +0,0 @@ ---- -bc-version: [all] -domain: performance -keywords: [countapprox, count, dialog, progress-bar, approximate-count] -technologies: [al] -countries: [w1] -application-area: [all] ---- - -# Use CountApprox for progress UI, not Count - -> Contributions welcome — open a PR to refine or extend this article. - -## Description - -`Count()` asks SQL for an exact row count of the current filter. When no SIFT key covers all filtered fields, this is a `SELECT COUNT(*)` against the data rows before any useful work starts — the usual cost of `Dialog.Open` with a percentage bar. (A filtered count on a table with a matching SIFT key is cheap, but SIFT coverage cannot be assumed for arbitrary filters.) `CountApprox()` exists for the progress-UI case: it returns a cheap estimate (partition stats / metadata), accurate enough for a progress denominator. Agents default to `Count()` because the name matches "how many rows". - -## Best Practice - -Feed progress dialogs and informational messages with `CountApprox()`. Use `Count()` only when the exact integer is a business result (a posted control, a reconciliation, a test assertion). - -See sample: `countapprox-for-progress-not-count.good.al`. - -## Anti Pattern - -`Total := Rec.Count(); Window.Open(...);` immediately before a `FindSet` over the same filter. The exact count is discarded after the bar finishes; when the filter is not covered by a SIFT key, the user paid a full table scan just to draw the progress bar. - -See sample: `countapprox-for-progress-not-count.bad.al`. diff --git a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al b/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al index b12a764..1a5c4aa 100644 --- a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al +++ b/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.bad.al @@ -11,14 +11,24 @@ page 50100 "GuiAllowed OData Guard Bad" repeater(Rows) { field("No."; Rec."No.") { } - field(Name; Rec.Name) { } + field(Name; Rec.Name) + { + StyleExpr = NameStyle; + } } } } + var + NameStyle: Text; + trigger OnAfterGetRecord() begin - // Runs for every OData / Edit-in-Excel row with no UI. + // UI-only styling still runs for every OData / Edit-in-Excel row. Rec.CalcFields("Balance (LCY)"); + if Rec."Balance (LCY)" > 0 then + NameStyle := 'Attention' + else + NameStyle := 'Standard'; end; } diff --git a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al b/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al index c638cf8..168afbe 100644 --- a/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al +++ b/community/knowledge/performance/guiallowed-guard-on-pages-used-as-odata.good.al @@ -11,15 +11,25 @@ page 50100 "GuiAllowed OData Guard Good" repeater(Rows) { field("No."; Rec."No.") { } - field(Name; Rec.Name) { } + field(Name; Rec.Name) + { + StyleExpr = NameStyle; + } } } } + var + NameStyle: Text; + trigger OnAfterGetRecord() begin if not GuiAllowed then exit; Rec.CalcFields("Balance (LCY)"); + if Rec."Balance (LCY)" > 0 then + NameStyle := 'Attention' + else + NameStyle := 'Standard'; end; } diff --git a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al b/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al index ea490fe..903fb2e 100644 --- a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al +++ b/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.good.al @@ -1,25 +1,62 @@ codeunit 50100 "HttpClient Holds Locks Good" { - // Write completes inside the caller's transaction; HTTP deferred so locks are released with it. procedure SyncCustomerLastName(var Customer: Record Customer) + var + CustomerSyncOutbox: Record "Customer Sync Outbox"; begin Customer."Search Name" := Customer.Name; Customer.Modify(false); - // RecordId binds the task to this specific customer; the platform loads it into Rec on OnRun. - TaskScheduler.CreateTask(Codeunit::"Customer Sync Task", 0, true, CompanyName(), CurrentDateTime(), Customer.RecordId); + + // This work item commits or rolls back with the customer change. + CustomerSyncOutbox."Customer No." := Customer."No."; + CustomerSyncOutbox.Insert(); end; } -codeunit 50101 "Customer Sync Task" +table 50100 "Customer Sync Outbox" { - TableNo = Customer; + DataClassification = CustomerContent; + + fields + { + field(1; "Entry No."; Integer) + { + AutoIncrement = true; + } + field(2; "Customer No."; Code[20]) { } + } + + keys + { + key(PK; "Entry No.") + { + Clustered = true; + } + } +} + +codeunit 50101 "Customer Sync Outbox Worker" +{ + // Configure this codeunit as a recurring job queue entry. + TableNo = "Job Queue Entry"; trigger OnRun() var + Customer: Record Customer; + CustomerSyncOutbox: Record "Customer Sync Outbox"; Client: HttpClient; Response: HttpResponseMessage; begin - // Separate session: Rec is the single customer passed via RecordId; no write-transaction lock is held. - Client.Get(StrSubstNo('https://example.local/sync/%1', Rec."No."), Response); + // Only committed work is visible here; a rolled-back change leaves no outbox row. + if not CustomerSyncOutbox.FindFirst() then + exit; + + Customer.Get(CustomerSyncOutbox."Customer No."); + Client.Get(StrSubstNo('https://example.local/sync/%1', Customer."No."), Response); + if not Response.IsSuccessStatusCode() then + Error('Customer sync failed with HTTP status %1.', Response.HttpStatusCode()); + + // Delete only after HTTP completes, so no write lock is held during the call. + CustomerSyncOutbox.Delete(); end; } diff --git a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md b/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md index c7ad656..4c1d500 100644 --- a/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md +++ b/community/knowledge/performance/httpclient-inside-write-transaction-holds-locks.md @@ -17,7 +17,9 @@ The first database write opens an AL write transaction that the runtime holds un ## Best Practice -Defer the HTTP call to a `TaskScheduler` task or job queue entry so it runs in a separate session after the write transaction has already ended. The database write completes and releases its locks naturally when the caller's transaction commits; the HTTP call then happens without holding any locks. Do **not** use `Commit()` as a general remedy: it irrevocably commits all prior writes in the current transaction, so any subsequent failure cannot roll them back. `Commit()` is appropriate only at top-level entry points where partial persistence is intentional and understood. +Defer the HTTP call to a separate session. When the external operation must correspond to a committed database change, insert an outbox work item in the same transaction as that change and process committed outbox rows with a recurring job queue entry. The change and work item then commit or roll back together, and the worker performs HTTP before deleting the item so it holds no write lock during the call. Make the external operation idempotent because a failure after a successful HTTP response can cause the work item to be retried. + +A directly created scheduled task is suitable only when its work is independent of the caller's commit. An immediately ready task can run concurrently with the caller, so it must not assume that the caller's writes are already committed. Do **not** use `Commit()` as a general remedy: it irrevocably commits all prior writes in the current transaction, so any subsequent failure cannot roll them back. `Commit()` is appropriate only at top-level entry points where partial persistence is intentional and understood. See sample: `httpclient-inside-write-transaction-holds-locks.good.al`. diff --git a/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.bad.al b/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.bad.al deleted file mode 100644 index 0ec934f..0000000 --- a/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.bad.al +++ /dev/null @@ -1,23 +0,0 @@ -page 50100 "Visible False Page Cost Bad" -{ - PageType = List; - SourceTable = Customer; - ApplicationArea = All; - - layout - { - area(content) - { - repeater(Rows) - { - field("No."; Rec."No.") { } - field(Name; Rec.Name) { } - // Hidden still participates in page load / FlowField calculation. - field("Balance (LCY)"; Rec."Balance (LCY)") - { - Visible = false; - } - } - } - } -} diff --git a/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.good.al b/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.good.al deleted file mode 100644 index da6f87f..0000000 --- a/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.good.al +++ /dev/null @@ -1,18 +0,0 @@ -page 50100 "Visible False Page Cost Good" -{ - PageType = List; - SourceTable = Customer; - ApplicationArea = All; - - layout - { - area(content) - { - repeater(Rows) - { - field("No."; Rec."No.") { } - field(Name; Rec.Name) { } - } - } - } -} diff --git a/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.md b/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.md deleted file mode 100644 index fc6281d..0000000 --- a/community/knowledge/performance/visible-false-does-not-skip-page-field-cost.md +++ /dev/null @@ -1,28 +0,0 @@ ---- -bc-version: [all] -domain: performance -keywords: [visible, enabled, page-field, list-page, metadata, hidden-control] -technologies: [al] -countries: [w1] -application-area: [all] ---- - -# Visible false does not skip page-field cost - -> Contributions welcome — open a PR to refine or extend this article. - -## Description - -`Visible = false` and `Enabled = false` hide a control; they do not remove it from the page metadata the client and server still process. List pages in particular still load bound fields and can still calculate FlowFields on those controls — see `hidden-flowfields-still-calculate-before-bc26-opt-in.md` for the FlowField-specific opt-in. Official page-performance guidance is to **delete** the field from the page object when users do not need it. Agents hide heavy columns instead of removing them. - -## Best Practice - -If a list or card should not pay for a column, omit the field from the page (or page extension) layout. Use `Visible` only for controls that must exist for some users or modes and whose cost is acceptable when hidden. Do not treat `Visible = false` as a performance fix. - -See sample: `visible-false-does-not-skip-page-field-cost.good.al`. - -## Anti Pattern - -Adding an expensive bound field or FlowField to a list and setting `Visible = false` "so it does not run". The control remains in the page definition. The signal is a hidden bound field whose only purpose was to avoid showing data, not to toggle a real mode. - -See sample: `visible-false-does-not-skip-page-field-cost.bad.al`.