Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf: use get_cached_value instead of db.get_value in controllers #32776

Merged
merged 8 commits into from
Nov 7, 2022
Merged

perf: use get_cached_value instead of db.get_value in controllers #32776

merged 8 commits into from
Nov 7, 2022

Conversation

DaizyModi
Copy link
Contributor

@DaizyModi DaizyModi commented Oct 31, 2022 •
edited by sagarvora
Loading

Replaced db.get_value with get_cached_value for masters doctype in Controllers file.

Before

In [2]: %timeit doc.validate_enabled_taxes_and_charges()
528 µs ± 3.3 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

After (without local cache)

In [3]: %timeit doc.validate_enabled_taxes_and_charges()
5.85 µs ± 64.2 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

Closes #32720

@github-actions github-actions bot added accounts needs-tests This PR needs automated unit-tests. stock labels Oct 31, 2022
@sagarvora sagarvora marked this pull request as ready for review November 1, 2022 10:29
@sagarvora sagarvora added squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. and removed needs-tests This PR needs automated unit-tests. labels Nov 1, 2022
@DaizyModi DaizyModi mentioned this pull request Nov 3, 2022
Merged
@ankush
Copy link
Member

ankush commented Nov 3, 2022 •
edited
Loading

@DaizyModi / @sagarvora make sure the numbers are without using local.cache{} between loops that timit executes (probably by writing simple test wrapper)

Otherwise it will be grossly misleading. I've seen simple queries + db.value_cache often being faster than get_cached_value.

Also db.value_cache is more reliable from cache eviction pov.

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #32776 (42f5858) into develop (7a5a500) will decrease coverage by 0.00%.
The diff coverage is 64.70%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #32776      +/-   ##
===========================================
- Coverage    63.81%   63.81%   -0.01%     
===========================================
  Files          817      817              
  Lines        58510    58508       -2     
===========================================
- Hits         37339    37337       -2     
  Misses       21171    21171
Impacted Files Coverage Δ
erpnext/controllers/trends.py 0.00% <0.00%> (ø)
erpnext/controllers/accounts_controller.py 84.88% <62.50%> (ø)
erpnext/controllers/stock_controller.py 92.08% <66.66%> (-0.04%) ⬇️
erpnext/controllers/sales_and_purchase_return.py 91.72% <100.00%> (ø)
erpnext/controllers/taxes_and_totals.py 95.05% <100.00%> (ø)

@deepeshgarg007 deepeshgarg007 merged commit 4efc947 into frappe:develop Nov 7, 2022
@deepeshgarg007
Copy link
Member

@Mergifyio backport version-14-hotfix version-13-hotfix

@mergify mergify bot mentioned this pull request Nov 7, 2022
Merged
mergify bot pushed a commit that referenced this pull request Nov 7, 2022
@DaizyModi @mergify
perf: use get_cached_value instead of db.get_value in controllers (…
34ca17a 
…#32776)

(cherry picked from commit 4efc947)
@mergify mergify bot mentioned this pull request Nov 7, 2022
Closed
mergify bot pushed a commit that referenced this pull request Nov 7, 2022
@DaizyModi @mergify
perf: use get_cached_value instead of db.get_value in controllers (…
fb1ddb8 
…#32776)

(cherry picked from commit 4efc947)

# Conflicts:
#	erpnext/controllers/stock_controller.py
@mergify
Copy link
Contributor

mergify bot commented Nov 7, 2022

backport version-14-hotfix version-13-hotfix

✅ Backports have been created

@sagarvora sagarvora deleted the fix-use-get-cached-value branch November 7, 2022 07:56
deepeshgarg007 added a commit that referenced this pull request Nov 7, 2022
@deepeshgarg007
Merge pull request #32870 from frappe/mergify/bp/version-14-hotfix/pr…
c2da8cf 
…-32776

perf: use `get_cached_value` instead of `db.get_value` in controllers (backport #32776)
frappe-pr-bot pushed a commit that referenced this pull request Nov 8, 2022
@frappe-bot
chore(release): Bumped to Version 14.6.0
e917212 
# [14.6.0](v14.5.1...v14.6.0) (2022-11-08)

### Bug Fixes

* `Material Consumption` option in case of `Skip Transfer to WIP` in WO ([8c856cd](8c856cd))
* add translate function to name of chart labels in budget_variance_report.py ([16f364d](16f364d))
* add translate function to name of chart labels in deferred_revenue_and_expense.py ([b8caa58](b8caa58))
* add translate function to period in  stock_analytics.py ([b0c06d5](b0c06d5))
* add translate function to period in sales_analytics.py ([e681f06](e681f06))
* add translate function to string on budget_variance_report.js to match the variance  word translated ([595aaad](595aaad))
* Auto advance allocation against partial invoices ([b7763d9](b7763d9))
* auto increment qty if item table has no items ([d8e403b](d8e403b))
* conflicts ([ab87a95](ab87a95))
* correct linters ([8f6f9a4](8f6f9a4))
* correct linters ([440e208](440e208))
* correct linters ([5acc9be](5acc9be))
* Create POS Opening Entry POS Profile filter. ([60af9c0](60af9c0))
* Disable tax included prices for internal transfers ([#32794](#32794)) ([6838e5e](6838e5e))
* for asset's purchase_date, if bill_date is set, use that instead of posting_date ([01a1c96](01a1c96))
* get `consumed_qty` based on `received_qty` in SCR ([ea9a502](ea9a502))
* Increase columns width in Warehouse wise Item Balance Age and Value ([0b09c31](0b09c31))
* linter ([af60c8f](af60c8f))
* make `BOM` required in SCR Item ([3f79a05](3f79a05))
* make `consumed_qty` editable when backflush based on Material Transfer ([2c5a8c4](2c5a8c4))
* make `consumed_qty` read-only in SCR Supplied Items ([68229f0](68229f0))
* map `BOM` while mapping the return SCR ([e629cba](e629cba))
* mysql syntax issue ([4d9bbd4](4d9bbd4))
* not able to select customer / supplier ([6989cdf](6989cdf))
* refactor code for better translatable string ([2dc24f2](2dc24f2))
* refactor code for better translatable string in stock_ageing.py ([0ead516](0ead516))
* rename test method ([97445d9](97445d9))
* Scan Barcode UX ([1944f4d](1944f4d))
* set `received_qty` before_validate SCR ([e316558](e316558))
* test cases ([0feec4c](0feec4c))
* trailing whitespace ([31bada9](31bada9))
* update advance paid in SO/PO from Payment Ledger ([a561432](a561432))
* use `flt` instead of `cint` in `get_batch_no` ([6510464](6510464))

### Features

* Item Wise TDS Calculation ([b9fb104](b9fb104))

### Performance Improvements

* use `get_cached_value` instead of `db.get_value` in controllers ([#32776](#32776)) ([34ca17a](34ca17a))
@barredterra barredterra mentioned this pull request Mar 7, 2023
Closed
@ankush ankush mentioned this pull request Oct 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagarvora sagarvora sagarvora left review comments

@deepeshgarg007 deepeshgarg007 Awaiting requested review from deepeshgarg007 deepeshgarg007 is a code owner

@nextchamp-saqib nextchamp-saqib Awaiting requested review from nextchamp-saqib

@rohitwaghchaure rohitwaghchaure Awaiting requested review from rohitwaghchaure rohitwaghchaure is a code owner

Assignees
No one assigned
Labels
accounts squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. stock
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

perf: Use get_cached_value instead of db.get_value for validate_enabled_taxes_and_charges
4 participants
@DaizyModi @ankush @deepeshgarg007 @sagarvora