Merge importFrom() calls to avoid loadNamespace() performance hit

[lionel-]

# Before
importFrom(stats,median)
importFrom(stats,sd)

# After
importFrom(
  stats,
  median,
  sd
)

I noticed that writing many importFrom() lines to a NAMESPACE file causes a linear performance hit in loadNamespace() (values in ms):

	N=10	N=100	N=1,000	N=10,000
importFrom() single	9	9	9	13
importFrom() many	9.5	18.5	119	1,619.5
export()	1	1	2	9
S3method() internal	1	2	7	56
S3method() foreign	1	3	20	427.5

loadNamespace() is complicated and fixing it upstream would be non-trivial. One approach that would work would be to coalesce contiguous runs of imports from the same package, which would fix the issue with Roxygen (since its generated namespace file is sorted) but not necessarily with manually written files.

Even if it were fixed upstream, it's probably worth avoiding the perf hit on older R versions, so I thought we'd go ahead and merge the import calls in our namespace file generation.

[hadley]

That's a pretty big performance hit!

I wonder if another approach would be to "compile" the NAMESPACE during the build process.

[hadley]

Is there a reason you tackled this here instead of creating a richer representation of NAMESPACE files so we can just generate directly?

[lionel-]

It seemed simpler at first, until I hit the raw-namespace edge cases. I'll take another look.

[hadley]

The other approach would be to parse the file since it's valid R code

[kevinushey]

One small behaviour change worth noting (probably acceptable): an import declared via both @importFrom and @rawNamespace with identical formatting now produces a duplicate line. The old code ran sort_c(unique(lines)) over all rendered directives, so these collapsed to one; the new code dedups the @rawNamespace text and dedups within the merge separately, but never compares the merged blocks against the raw text.

#' @importFrom stats median
#' @rawNamespace importFrom(stats,median)
NULL
#> main: "importFrom(stats,median)"
#> PR:   "importFrom(stats,median)" "importFrom(stats,median)"

Narrow (requires the same import both ways with matching formatting), but it does emit a duplicate directive. Easy to dedup the merged blocks against text in ns_format() if you decide it matters.

[hadley]

@kevinushey I think it's fine to basically ignore the contents of @rawNamespace. It's pretty rarely used and in general should only be for namespace syntax that roxygen2 doesn't otherwise support.

@lionel- I played around with a parsing based approach but ultimately didn't like. Instead I've added a minimally richer representation to the namespace data structure. I also changed the style to emit the package name on the first line which I think makes it clearer that the first argument is special. (This does not match our code style, but I think it's ok here since the NAMESPACE is only nominally R code.)