Skip to content

Commit

Permalink
[report-utilities] more efficient gnc:accounts-get-commodities
Browse files Browse the repository at this point in the history
don't create intermediate lists which are immediately used and
discarded.

don't locale-compare commodity names, which has locale-transform
overhead, and creates lots of temporary strings.

testing for duplicates via O(N) member is very adequate because
num(unique commodities) is expected to be small.

use the stack when accumulating commodities, avoiding need to prepend
and reverse.
  • Loading branch information
christopherlam committed Jan 11, 2024
1 parent 7a17b24 commit 1a5247c
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 21 deletions.
7 changes: 1 addition & 6 deletions gnucash/report/commodity-utilities.scm
Original file line number Diff line number Diff line change
Expand Up @@ -229,12 +229,7 @@
(gnc-account-get-descendants-sorted (gnc-get-current-root-account)))
(all-splits (get-all-splits currency-accounts end-date))
(interesting-splits (sort (filter interesting-split? all-splits) date<?))
(commodity-list (sort-and-delete-duplicates
commodity-list
(lambda (a b)
(gnc:string-locale<? (gnc-commodity-get-unique-name a)
(gnc-commodity-get-unique-name b)))
gnc-commodity-equal))
(commodity-list (delete-duplicates commodity-list))
(work-to-do (length commodity-list)))
(map
(lambda (c work-done)
Expand Down
13 changes: 5 additions & 8 deletions gnucash/report/report-utilities.scm
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,11 @@
;; Get the list of all different commodities that are used within the
;; 'accounts', excluding the 'exclude-commodity'.
(define (gnc:accounts-get-commodities accounts exclude-commodity)
(delete exclude-commodity
(sort-and-delete-duplicates
(map xaccAccountGetCommodity accounts)
(lambda (a b)
(gnc:string-locale<? (gnc-commodity-get-unique-name a)
(gnc-commodity-get-unique-name b)))
gnc-commodity-equiv)))

(if (null? accounts)
'()
(let ((comm (xaccAccountGetCommodity (car accounts)))
(accum (gnc:accounts-get-commodities (cdr accounts) exclude-commodity)))
(if (or (equal? exclude-commodity comm) (member comm accum)) accum (cons comm accum)))))

;; Returns the depth of the current account hierarchy, that is, the
;; maximum level of subaccounts in the tree
Expand Down
7 changes: 1 addition & 6 deletions gnucash/report/reports/standard/balsheet-pnl.scm
Original file line number Diff line number Diff line change
Expand Up @@ -739,12 +739,7 @@ also show overall period profit & loss."))
;; generate an exchange-fn for date, and cache its result.
(get-date-exchange-fn
(let ((h (make-hash-table))
(commodities (sort-and-delete-duplicates
(map xaccAccountGetCommodity accounts)
(lambda (a b)
(gnc:string-locale<? (gnc-commodity-get-unique-name a)
(gnc-commodity-get-unique-name b)))
gnc-commodity-equal)))
(commodities (gnc:accounts-get-commodities accounts #f)))
(lambda (date)
(or (hashv-ref h date)
(let ((exchangefn (gnc:case-exchange-time-fn
Expand Down
2 changes: 1 addition & 1 deletion gnucash/report/reports/standard/test/test-balsheet-pnl.scm
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@
'("#200.00" "$340.00" "30 FUNDS" "$14,424.52" "$106,709.00" "$106,709.00")
(sxml->table-row-col sxml 1 3 6))
(test-equal "show-rates enabled"
'("#1.00" "$1.7000" "1 FUNDS" "$480 + 85/104")
'("1 FUNDS" "$480 + 85/104" "#1.00" "$1.7000")
(sxml->table-row-col sxml 2 #f #f)))

;;make-multilevel
Expand Down

2 comments on commit 1a5247c

@christopherlam
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing for duplicates via O(N) member is very adequate because
num(unique commodities) is expected to be small.

Oops testing for duplicates is O(n^2) but it's still ok because nobody will use gnucash to handle hundreds of commodities.

@jralls
Copy link
Member

@jralls jralls commented on 1a5247c Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops testing for duplicates is O(n^2) but it's still ok because nobody will use gnucash to handle hundreds of commodities.

I don't think you can count on that, and you wrote sort-and-delete-duplicates explicitly to avoid delete-duplicates O(n^2) problem.

OTOH this

(let ((comm (xaccAccountGetCommodity (car accounts)))
            (accum (gnc:accounts-get-commodities (cdr accounts) exclude-commodity)))
        (if (or (equal? exclude-commodity comm) (member comm accum)) accum (cons comm accum)))))

looks like a good improvement over collecting all of the commodities, sorting, deleting the dupes, and then deleting the exclude.

Please sign in to comment.