knowledge(performance): add community rules for performance - #134
Open
Stefano Demiliani (demiliani) wants to merge 3 commits into
Open
knowledge(performance): add community rules for performance#134Stefano Demiliani (demiliani) wants to merge 3 commits into
Stefano 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>
Comment on lines
+9
to
+16
|
|
||
| # 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.
Contributor
|
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
Contributor
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.
Author
|
Updated PR submitted with all the remaining suggested fixes. |
Jesper Schulz-Wedde (JesperSchulz)
approved these changes
Aug 21, 2026
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Contributor
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 🎉
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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".