Subscription Billing: improve performance for large datasets#7776
Subscription Billing: improve performance for large datasets#7776jeffreybulanadi wants to merge 2 commits into
Conversation
…rosoft#7690) Root causes addressed: 1. Missing database indexes causing full table scans on 4M+ billing lines 2. No progress dialog in billing proposal creation (unusable at scale) 3. Window.Open/Close/Update calls not guarded with GuiAllowed (Job Queue failures) 4. GetBillingLineNo() called inside UsageDataBilling inner loop (N^2 queries) 5. CustomerContract/VendorContract.Get() called for every billing line (millions of round-trips) 6. No per-document Commit() leaving entire run as one giant transaction Changes: - BillingLine.Table.al: add SK7 (Partner, ContractNo, ContractLineNo, DocType, DocNo) covering the GetBillingLineNo() and UsageDataBilling query patterns - SubscriptionLine.Table.al: add Key4 (Partner, ContractNo, NextBillingDate) covering the ProcessContractServiceCommitments filter pattern - UsageDataBilling.Table.al: add key2 (Partner, ContractNo, ContractLineNo, DocType, DocNo) enabling efficient Usage Based Billing document-save lookups - UsageDataGenericImport.Table.al: add key2 (UsageDataImportEntryNo, ProcessingStatus) enabling efficient status-filtered queries on import lines - BillingProposal.Codeunit.al: add progress dialog (BillingProposalProgressTxt) showing contract no., partner no., and processed/total count; guard all Message/Page.Run with if GuiAllowed to allow Job Queue execution - CreateBillingDocuments.Codeunit.al: - Wrap all Window.Open/Close/Update with if GuiAllowed - Wrap ProcessingFinishedMessage with if GuiAllowed - Add Commit() after each completed billing document in all four document-creation procedures (PerContract/PerCustomer for Sales; PerContract/PerVendor for Purchase) to create recovery points and reduce transaction size - Hoist GetBillingLineNo() call outside the UsageDataBilling FindSet() loop in both InsertSalesLineFromTempBillingLine and InsertPurchaseLineFromTempBillingLine; the result is constant within a given document line so calling it once is correct - Cache CustomerContract.Get() and VendorContract.Get() in CreateTempBillingLines using a last-seen contract no. variable; reduces O(n) Get() calls to O(distinct contracts) with zero behavioral change - Extend ProgressTxt label with a third field showing contracts-processed count Fixes microsoft#7690
github-actions
Bot
added
AL: Apps (W1)
jeffreybulanadi
changed the title
There was a problem hiding this comment.
This looks like a great work. I have some more suggestions for improvements and few more questions:
- Usage Data Import processing was not improved and is further lacking the progress windows. This is possible to be done at some later point if the speedup is desired.
- Unnecessary validation chains during line creation - was not addressed. Could be done in follow-up.
- The performance bottleneck problem in CheckServiceCommitmentDataConsistency was not addressed - Could be done in follow-up.
- Line numbering via FindLast() instead of in-memory counters - Could be done in follow-up.
- I know it is out of the current scope, but I would at least in some of the follow ups display some more information in the Dialog Window. The praxis has shown that a user would like to know what the average time is spent on a contract or document during document creation and billing proposal and perhaps the estimated remaining time based on the contract or document counter.
| key(SK7; Partner, "Subscription Contract No.", "Subscription Contract Line No.", "Document Type", "Document No.") | ||
| { | ||
| } |
There was a problem hiding this comment.
Where does these Key kicks in? Partner is an Enum field and thus has low cardinality. If filtered on all fields from this key, I would consider either moving this field after other fields with high cardinality or removing the low cardinality fields from the key completely.
| key(Key4; Partner, "Subscription Contract No.", "Next Billing Date") | ||
| { | ||
| } |
There was a problem hiding this comment.
Suggestion to consider skipping low cardinality fields in first place of the key.
| } | ||
| key(key2; Partner, "Subscription Contract No.", "Subscription Contract Line No.", "Document Type", "Document No.") | ||
| { | ||
| } |
There was a problem hiding this comment.
Suggestion to consider skipping low cardinality fields in the first place of the key.
Some more key suggesions:
"Usage Data Import Entry No.", "Processing Status" - low cardinality for "Processing Status.
"Usage Data Import Entry No.", "Document No."
…y fields - BillingLine SK7: move Subscription Contract No. and Subscription Contract Line No. before Document Type and Partner. Contract No. is a Code[20] field with high cardinality and narrows the result set far more efficiently than the Partner enum (2 values) as the leading column. - SubscriptionLine Key4: move Subscription Contract No. before Partner and Next Billing Date. ProcessContractServiceCommitments always filters by Contract No., making it the most selective leading field. - UsageDataBilling key2: same rationale as SK7. Subscription Contract No. leads, followed by Contract Line No. and Document No. (both high cardinality), with Document Type and Partner at the end. - UsageDataBilling key3: add index on Usage Data Import Entry No. and Document No. to support lookups that associate import records with billing documents.
|
Good catch on the key ordering. Partner is an enum with two values (Customer/Vendor), and leading a composite index with it means 50% of the index range is traversed before the more selective fields narrow the scan. Same issue applies to Document Type. The keys have been reordered to lead with Subscription Contract No. (Code[20], high cardinality) across all three tables:
Also added UsageDataBilling key3 on (Usage Data Import Entry No., Document No.) per the suggestion in the last comment. Document No. carries higher cardinality than Processing Status, so it makes a more selective second column for entry-level lookups. |
| key(SK6; "Billing Template Code", Partner) | ||
| { | ||
| } | ||
| key(SK7; "Subscription Contract No.", "Subscription Contract Line No.", "Document No.", "Document Type", Partner) |
There was a problem hiding this comment.
Adding new keys can be costly and may negatively impact performance. Based on the analysis, this key degrades performance for some operations, so I recommend removing it.
There was a problem hiding this comment.
Good point. The function GetBillingLineNo might be a bottleneck indeed which I already observed on the real project. Instead of adding the key I suggest removing the function GetBillingLineNo completely and passing the "Billing Line No." as a denormalized field. That would bring more benefit to the performance then the key.
|
@JesperSchulz @djukicmilica Can I please create another PR that contain remaining 50% percent of optimizations I was planning to do? This original issue was created by me after all, and I would like to avoid additional effort for the second run. |
Yes, please. :) |
|
Hi @jeffreybulanadi , can you please check the comments? :) |
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR improves Subscription Billing for large datasets by adding table keys, adding progress dialogs, guarding dialog use with SuggestionsS1 - Make the W1 build pass S2 - Add a transaction regression test S3 - Add automated no-GUI coverage S4 - Resolve the overlap with PR 8553 Risk assessment and necessityRisk: The highest risk is the new commit boundary in Necessity: The large-dataset scenario is valid and important; issue #7690 describes runs with about 4 million subscription lines and missing Job Queue support. The scope is still incomplete for that issue: usage import progress, validation-chain reduction, in-memory line numbering, and the
|
|
Hi @jeffreybulanadi , can you please check the comments? :) |
Hi @djukicmilica, I have created another PR. I asked for this change since I have had (a bad) experience from a large scare project. Can we please close this PR in favor of my PR? #8553 |
5 participants
Fixes #7690
Summary
Subscription Billing document creation is unscalable at 4M+ subscription lines due to missing indexes, crashes in Job Queue, an O(n2) query pattern, redundant database reads, and no per-document transaction boundary. This PR addresses all root causes identified in the issue.
Root Causes Fixed
1. Missing database indexes
BillingLine: Added keySK7on(Partner, Subscription Contract No., Subscription Contract Line No., Document Type, Document No.)coveringGetBillingLineNo()on every line insertionSubscriptionLine: Added keyKey4on(Partner, Subscription Contract No., Next Billing Date)coveringProcessContractServiceCommitments()UsageDataBilling: Added keykey2on(Partner, Subscription Contract No., Subscription Contract Line No., Document Type, Document No.)mirroring the billing line lookup patternUsageDataGenericImport: Added keykey2on(Usage Data Import Entry No., Processing Status)covering status-filtered scans during import2. Job Queue crash (GuiAllowed = false)
BillingProposal:ProposalWindow.Open/Update/Closenow guarded withif GuiAllowed. TheCount()call that feeds the progress dialog is also skipped whenGuiAllowed = false.CreateBillingDocuments:Window.Open/Update/Closealready had anAutomatedBillingguard but was missingGuiAllowedchecks. Both guards are now applied.ProcessingFinishedMessageexits early whennot GuiAllowed.3. Redundant GetBillingLineNo() calls inside inner loop
InsertSalesLineFromTempBillingLineandInsertPurchaseLineFromTempBillingLine,GetBillingLineNo()was called inside aUsageDataBilling.FindSet()loop with constant parameters. Hoisted outside the loop.4. Redundant contract Get() on every billing line
CreateTempBillingLinescalledCustomerContract.Get()andVendorContract.Get()for every billing line. Added cache variablesCachedCustomerContractNoandCachedVendorContractNoto skip the read when the contract has not changed.5. Single transaction for entire batch
CreateSalesDocumentsPerContract,CreatePurchaseDocumentsPerContract,CreateSalesDocumentsPerCustomer, andCreatePurchaseDocumentsPerVendoreach callCommit()after completing each document. This limits the transaction scope to one document at a time, preventing lock escalation and enabling partial-batch recovery.6. Progress visibility in BillingProposal
Dialogprogress window inCreateBillingProposalso users see per-contract progress during long proposal creation runs. Includes contract number, partner number, and a processed/total counter.Files Changed
src/Apps/W1/Subscription Billing/App/Billing/Tables/BillingLine.Table.al-- added SK7src/Apps/W1/Subscription Billing/App/Service Commitments/Tables/SubscriptionLine.Table.al-- added Key4src/Apps/W1/Subscription Billing/App/Usage Based Billing/Tables/UsageDataBilling.Table.al-- added key2src/Apps/W1/Subscription Billing/App/Usage Based Billing/Tables/UsageDataGenericImport.Table.al-- added key2src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al-- progress dialog + GuiAllowed guardssrc/Apps/W1/Subscription Billing/App/Billing/Codeunits/CreateBillingDocuments.Codeunit.al-- GuiAllowed guards, Commit() per document, hoisted GetBillingLineNo(), cached contract Get()How to Test
Fixes AB#634052