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.

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

Remove cartesian product in MkConstBindingPathComponentList #161

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

lcartey
Copy link
Contributor

@lcartey lcartey commented Nov 27, 2024

We have observed performance issues in the UI5 queries related to bindings on databases with large amount of JSON (notably databases with significant numbers of CDS json file). Reviewing one such example, I observed the following slow predicates:

Most expensive predicates for completed query UI5Xss.ql:
        time  | evals |  max @ iter | predicate
        ------|-------|-------------|----------
        11.1s |     9 | 9.2s @ 1    | Bindings::BindingStringParser::MkConstBindingPathComponentList#dom#8f6dc5b2@f03adz6n
         9.2s |     6 | 7.1s @ 2    | num#Bindings::BindingStringParser::MkConstBindingPathComponentList#173021d1@f03adw6n
         8.5s |     2 | 6.9s @ 1    | Bindings::BindingStringParser::BindingPathComponentList.toString/0#2692bb01@49b354t2
         6.5s |     7 | 6.1s @ 3    | Bindings::BindingStringParser::MkConstBindingPathComponentList#173021d1#reorder_1_0_2_3@f03adx6n
         1.7s |     8 | 1.7s @ 4    | Bindings::BindingStringParser::mkBindingPathComponentList/3#6ec81d06#reorder_1_0_2@f03ady6n
         1.4s |     9 | 1.3s @ 4    | _Bindings::BindingStringParser::MkConstBindingPathComponentList#173021d1#reorder_1_0_2_3#prev_delta___#antijoin_rhs@L0#f03ad

Reviewing, the cause was that source was not bound within MkConstBindingPathComponentList.

    MkConstBindingPathComponentList(NameToken headToken, BindingPathComponentList tail, Token source) {
      exists(Token nextToken | nextToken = getNextSkippingWhitespace(headToken) |
        if nextToken instanceof ForwardSlashToken or nextToken instanceof DotToken
        then mkBindingPathComponentList(getNextSkippingWhitespace(nextToken), tail, _)
        else tail = MkEmptyBindingPathComponentList()
      )
    }

This was creating a cross-product with all source Tokens in the program (all tokens in potential binding paths).

As source was unused in any meaningful way outside the class itself, we can remove it without impact.

In addition, we can restrict the search for binding paths for XML and JSON files to those files which we actually consider to be views for UI5.

`source` was not bound within MkConstBindingPathComponentList, and
was semantically unused outside the class itself.
This ensures we don't look for UI5 bindings in, for example,
cds.json files.
Copy link
Contributor

@knewbury01 knewbury01 left a comment

Choose a reason for hiding this comment

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

@lcartey need to address the testcase failures, thanks!

 - Ensure HTML views are covered within TBindingStrings
 - Update test case to use a valid html view (.view.html)
All XML, HTML and JSON views should have a .view.<extension>
extension format.
@lcartey lcartey requested a review from knewbury01 November 28, 2024 11:39
Copy link
Contributor

@knewbury01 knewbury01 left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for the improvement

@knewbury01 knewbury01 merged commit 0ea21ec into main Dec 2, 2024
5 checks passed
@knewbury01 knewbury01 deleted the lcartey/remove-binding-path-cartesian-product branch December 2, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants