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

Move symbol search into XPath in backport_linter #2365

Closed
wants to merge 2 commits into from
Closed

Conversation

MichaelChirico
Copy link
Collaborator

For #2338. This approach presents a bit of a conundrum as it seems the performance of the XPath algorithm degrades with the number of objects to search for:

(new: this PR; old: current main. 413: backport_linter("4.1.3"); 300: backport_linter("3.0.0"))

system.time(lint_all(f, old413))
#    user  system elapsed 
#  71.890   1.442  74.399 
system.time(lint_all(f, old300))
#    user  system elapsed 
#  70.846   1.036  72.075

system.time(lint_all(f, new413))
#    user  system elapsed 
#  65.206   0.711  66.198 
system.time(lint_all(f, new300))
#    user  system elapsed 
#  76.417   0.854  77.541 

i.e. we get about 10% improvement from the "easier" linter where there are fewer objects to look up, but 7% slowdown in the "harder" case with many objects to look up.

Not sure how to proceed. We may also want to check other linters that use a huge list of or checks & check alternate approaches.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6d2b327) 99.13% compared to head (abaa734) 99.13%.

❗ Current head abaa734 differs from pull request most recent head 07ceea5. Consider uploading reports for the commit 07ceea5 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2365   +/-   ##
=======================================
  Coverage   99.13%   99.13%           
=======================================
  Files         124      124           
  Lines        5573     5573           
=======================================
  Hits         5525     5525           
  Misses         48       48           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AshesITR
Copy link
Collaborator

Sounds like #2357 promises better performance than this approach in the long run.
Table until that PR is ready?

@MichaelChirico
Copy link
Collaborator Author

Sounds like #2357 promises better performance than this approach in the long run. Table until that PR is ready?

It will still need a //SYMBOL search, it may be cumbersome to have a linter using both the //SYMBOL_FUNCTION_CALL cache and a general XPath search.

Anyway it does seem like a broader issue, but my instinct is the affected cases will mostly be looking at //SYMBOL_FUNCTION_CALL, so makes sense to pause.

@MichaelChirico MichaelChirico marked this pull request as draft November 29, 2023 21:36
@MichaelChirico
Copy link
Collaborator Author

BTW, this at least partially seems related to our XPath1.0 restriction, it would be great to to have an XPath2.0+ backend but I don't see any R support for that. LLM turned up this C++ library XQilla though, a long-term project could be to write an R package wrapping this with the same/similar API as {xml2}.

@AshesITR
Copy link
Collaborator

NB: backport_linter() is converted in #2357 now.

AshesITR added a commit that referenced this pull request Dec 13, 2023
@AshesITR
Copy link
Collaborator

This idea was implemented in #2357 as well now, so closing this PR.

@AshesITR AshesITR closed this Dec 13, 2023
MichaelChirico added a commit that referenced this pull request Dec 13, 2023
* add first implementation of xml_find_function_calls

* delint

* support getting all function calls and using names.

* squash conversions

* review comments

* clean up

* add vignette section and NEWS bullet

* smarter conjunct_test_linter migration

* smarter consecutive_assertion_linter migration

* remove self::SYMBOL_FUNCTION_CALL[text() = ...] xpaths

* delint

* Update expect_s3_class_linter.R

* Reference GH issue # in TODO

* review comments

* add missing comma in example

* Update NEWS.md

* supersede #2365

* Update R/xp_utils.R

Co-authored-by: Michael Chirico <[email protected]>

* fix bad commit

* Update NEWS.md

* Add an upper bound improvement from r-devel

---------

Co-authored-by: Michael Chirico <[email protected]>
Co-authored-by: Michael Chirico <[email protected]>
@MichaelChirico MichaelChirico deleted the backort branch December 27, 2023 09:17
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.

3 participants