improve handling of glued object extraction #1612
Conversation
|
Benchmark-ish. Did a find-and-replace in one dplyr file for # on main
system.time(replicate(40, simplify=FALSE, lint("dplyr/R/join-by.R", object_usage_linter())))
# user system elapsed
# 63.096 0.331 63.701
# on this branch
system.time(replicate(40, simplify=FALSE, lint("dplyr/R/join-by.R", object_usage_linter())))
# user system elapsed
# 59.484 0.204 59.891So a small speed-up (5-6%) in typical usages. It should be a lot better in pathological cases (especially a |
| if (is.null(parsed_text)) { | ||
| return("") | ||
| } | ||
| parse_data <- utils::getParseData(parsed_text) | ||
| # covers NULL & NA cases | ||
| if (nrow(parse_data) == 0L) { | ||
| return("") | ||
| } |
There was a problem hiding this comment.
Are there existing tests indirectly covering the two conditionals here?
There was a problem hiding this comment.
I defer to codecov :)
| if (nrow(parse_data) == 0L) { | ||
| return("") | ||
| } | ||
| symbols <- parse_data$text[parse_data$token == "SYMBOL"] |
There was a problem hiding this comment.
Does this approach differentiate between x and `x`?
There was a problem hiding this comment.
Hmm I think it doesn't, but neither did the old approach.
Codecov Report
@@ Coverage Diff @@
## main #1612 +/- ##
==========================================
- Coverage 95.07% 95.06% -0.02%
==========================================
Files 98 98
Lines 4390 4393 +3
==========================================
+ Hits 4174 4176 +2
- Misses 216 217 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
.transformerfunction outside thefor (cl in glue_calls)loop.envirenvironment to remove the dependency of the.transformeron globals insideextract_glued_symbolsgetParseData()output for this symbol case rather than converting to XML and then searching an XPathnames(env)overls(env, all.names=TRUE)which I found is somehow 20x fasterI tried testing the performance on
dplyr, but it's mostly usingglue()unqualified, so I haven't had any luck yet. We might try loosening that restriction.