knowledge(performance): add community rules for performance - #134
knowledge(performance): add community rules for performance#134Stefano Demiliani (demiliani) wants to merge 3 commits into
Conversation
… positives These articles capture BC-specific mechanics agents still invert: partial-record JIT on writes, Reset clearing SetLoadFields, HttpClient inside write transactions, and batched number series, plus negative guidance that stops over-eager Query and IsEmpty "fixes". Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
||
| # 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. On a large table that is a `SELECT COUNT(*)` before any useful work starts — the usual cost of `Dialog.Open` with a percentage bar. `CountApprox()` exists for that 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". |
There was a problem hiding this comment.
Are you sure about CountApprox? It is used in BCApps only 14 times, and not once for a progress bar.
If I remember correctly (but I am really not sure), it was said years ago that it does not provide that much value anymore. But it used to be important and useful in the past.
There was a problem hiding this comment.
Count() executes a precise database scan to return the exact number of records, whereas CountApprox() pulls from SQL Server statistics to return an estimated number significantly faster. In Progress Bar (UI) on large tables it’s faster. But yes, we’re talking about ms on rare cases now. The rule can also not be mandatory.
There was a problem hiding this comment.
Natalie is correct. The intention was that CountApprox() should be faster than Count(). And it often was. Unfortunately the 'approx'-part meant that it sometimes returned 0 even if there were records. That meant two things: 1) We couldn't use the result for deciding whether to process anything or not, and 2) when we want to show progress as %, it meant that we potentially risked a division by zero, which again meant that we had to add extra code lines to guard against it.
Anyways: I just checked with our platform guys, and they tell me that CountApprox() calls the same code as Count() and has done so for a decade or so.
|
I will get the eyes of some of our performance experts on this 🙂 |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
High-confidence correctness findings only; community-layer placement is not a concern for this review.
Fixes technical inaccuracies and behavioral issues raised during review of the community performance knowledge articles. avoid-currpage-update-in-onaftergetrecord.md - Removed OnAfterGetCurrRecord from the Best Practice section. That trigger is itself implicitly re-entered on every refresh, so placing CurrPage.Update(false) inside it can re-trigger the very loop the rule warns about. Only OnAction remains as the recommended location. batch-number-series-instead-of-getnextno-per-row.md - Scoped the lock/contention claim to gapless (Normal) series only. Added explicit statement that Allow Gaps series use NumberSequence sequences and do not hold the series-line lock, so the anti-pattern detection signal now excludes Allow Gaps series. countapprox-for-progress-not-count.bad.al / .good.al - Changed FindSet(true) to FindSet() in both samples. The loop is read-only; UpdLock is not needed and was misleading. countapprox-for-progress-not-count.md - Qualified the Count() cost claim: it is expensive only when no SIFT key covers all filtered fields (forcing a SELECT COUNT(*)); a filtered count with matching SIFT coverage is cheap. Added a parenthetical noting that SIFT coverage cannot be assumed for arbitrary filters. dataaccessintent-readonly-on-analytical-objects.md - Changed bc-version from [all] to ["16.."]. DataAccessIntent was introduced at runtime 5.0 / BC 16 and has no effect in earlier versions. - Added precision to the supported objects: pages must be PageType=API with Editable=false; for queries, replica routing only applies when the query is exposed via OData/API, not for AL-to-AL calls. httpclient-inside-write-transaction-holds-locks.good.al - Replaced the Commit()-before-HttpClient pattern with a two-codeunit task-deferral pattern. The write completes inside the caller's transaction (locks released naturally when it ends); a TaskScheduler task runs the HTTP call in a separate session where no write- transaction lock is held. httpclient-inside-write-transaction-holds-locks.md - Changed Best Practice to recommend TaskScheduler/job queue deferral as the primary remedy. - Added an explicit warning against Commit() as a generic remedy: it irrevocably commits all prior writes in the current transaction, so a subsequent failure cannot roll them back. Commit() is appropriate only at top-level entry points where partial persistence is intentional. oncompanyopen-subscribers-must-not-do-io.good.al - Added a ClientType guard so the subscriber exits immediately in background task sessions (OnAfterLogin fires there too, which would create an unbounded task chain without the guard). - Added a TaskScheduler.TaskExists idempotency check to avoid queuing duplicate tasks on repeated logins. - Fixed the error-fallback codeunit in CreateTask from a self-reference to 0 (no error codeunit). - Added a 60-second delay (CurrentDateTime() + 60000) so the task does not compete with the login session itself. prefer-related-table-over-extension-on-hot-ledgers.md - Changed bc-version from [all] to ["23.."]. The companion-table join optimisation (single join per base table, automatic exclusion on List/ OData pages with partial records) was introduced in v23. - Scoped the "join is always paid" claim: since v23 the join is excluded on List/ListPart/OData pages when no extension field is loaded under partial-record semantics, but it is still paid on every posting path and any AL code that accesses an extension field. skip-setloadfields-on-write-and-transferfields.bad.al / .good.al / .md - Changed the example scenario from Modify(false) (which is actually valid with a partial record) to TransferFields+Insert into a temporary record, which is a documented full-load operation. - Removed Modify from the list of operations that force a full load. - Added an explicit note in the .md that Modify itself is not in the full-load list; SetLoadFields is safe to use before Modify(false). use-dedicated-lookup-pages-not-full-lists.bad.al - Added a separate CardPart page definition (50101) to replace the self-referencing FactBox part that referenced the same list page it was embedded in. A part cannot refer to its own container page. validate-on-partial-record-forces-jit.good.al - Restored SetLoadFields to the good sample so it exercises a partial record and the contrast with .bad.al is field selection, not the absence of the feature. The good sample loads both Name and Search Name (the field Name.OnValidate writes), while the bad sample loads only Name, causing a JIT reload of Search Name on every Validate call.
… samples httpclient-inside-write-transaction-holds-locks.good.al - Pass Customer.RecordId as the last argument to CreateTask so the task is bound to the single customer that was written, not to all customers. - Declare TableNo = Customer on the task codeunit so the platform loads the bound record into Rec automatically when OnRun executes. - Replace the FindSet loop over all customers with Rec."No.", preserving the one-customer scope of the original SyncCustomerLastName procedure. oncompanyopen-subscribers-must-not-do-io.good.al - Replace Session.GetCurrentClientType() with Session.CurrentClientType(), the correct platform method name. - Fix the idempotency check: TaskScheduler.TaskExists() requires a Guid, not a codeunit integer ID. Store the Guid returned by CreateTask in IsolatedStorage (DataScope::Company) under a fixed key; on the next login read it back as Text, Evaluate it to Guid, and pass that Guid to TaskExists so the type matches the method signature.
|
Updated PR submitted with all the remaining suggested fixes. |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Nice work! I think a few of these should get promoted to the Microsoft layer, but let's get them in first. I'll take a look all up at the community layer once a month I think, and pull the strongest articles one layer up 🎉
| exit(Customer.Name); | ||
| end; | ||
|
|
||
| procedure NamesForCompanies(var Company: Record Company) |
There was a problem hiding this comment.
Not sure why this pattern is better regarding change company - other than the SetLoadFields, which perhaps could have been set outside the loop. Is the function NameInCompany used?
| Total: Integer; | ||
| begin | ||
| Customer.SetRange("Country/Region Code", 'US'); | ||
| Total := Customer.CountApprox(); |
There was a problem hiding this comment.
Same as count()
| begin | ||
| if not GuiAllowed then | ||
| exit; | ||
| Rec.CalcFields("Balance (LCY)"); |
There was a problem hiding this comment.
Would this ever be good when we don't expose the balance field anyway?
| 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); |
There was a problem hiding this comment.
Generally, we should not lock while we do lengthy stuff like calling webservices etc.
In this concrete example you may risk starting a sync to before the data is committed, so maybe you sync the new (uncommitted) value or maybe you sync the old name. And if the transaction rolls back due to some error, you already made sync call. So perhaps syncs to external systems should happen after data is committed?
| CustomerByNo: Query "Query Bypass PK Cache Bad Q"; | ||
| begin | ||
| // Query Open/Read never hits the server PK cache. | ||
| CustomerByNo.SetRange(NoFilter, CustomerNo); |
There was a problem hiding this comment.
True. But an even simpler example that also omits the PKC is Customer.FindFirst()
|
|
||
| ## 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. |
There was a problem hiding this comment.
Well; in newer versions of BC (and also in old C/SIDE client, if anyone still uses it) only visible flowfields are calculated. By removing the expense field from the page you also prevent the user from showing it.
4 participants
Community rules for performance checks (missing in Microsoft standard):
partial-record JIT on writes, Reset clearing SetLoadFields, HttpClient inside write transactions, and batched number series, plus negative guidance that stops over-eager Query and IsEmpty "fixes".