mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
fix(community/performance): address review feedback
This commit is contained in:
Stefano Demiliani
parent
8373bc4717
commit
6a75a425af
13 changed files with 79 additions and 165 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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`.
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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`.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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`.
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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) { }
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -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`.
|
||||
Loading…
Add table
Add a link
Reference in a new issue