[PATCH] D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC

[Andrew Ng via Phabricator via llvm-commits]

andrewng marked 7 inline comments as done.
andrewng added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:324
+static StringRef getFilename(const InputFile *file) {
+  return file ? file->getNameForScript() : StringRef();
+}
----------------
MaskRay wrote:
> grimar wrote:
> > andrewng wrote:
> > > grimar wrote:
> > > > It feels that `getFilename` can instead be just inlined now, though?
> > > I've inlined the function.
> > I've meant it is a single line function and you can probably just remove it and inline its logic.
> > E.g:
> > ```
> >   if (!matchesFileCache || matchesFileCache->first != file)
> >     matchesFileCache.emplace(
> >         file, filePat.match(getFilename(file ? file->getNameForScript() : "")));
> > ```
> > Up to you.
> > 
> > 
> > A bit unrelated to this diff: thinking more I wonder if we want to assume an empty file name at all for matching.
> > We perhaps can be more explicit. Something like:
> > 
> > ```
> > bool InputSectionDescription::matchesFile(const InputFile *file) const {
> >   if (filePat.isTrivialMatchAll())
> >     return true;
> > 
> >   if (!file)
> >     return false;
> > ...
> > ```
> > 
> > ```
> > bool SectionPattern::excludesFile(const InputFile *file) const {
> >   if (!file || excludedFilePat.empty())
> >     return false;
> > ...
> > ```
> getFilename is called twice. Having it separate looks good to me.
> 
> > if (filePat.isTrivialMatchAll())
> 
> Avoiding the single-element cache with just `(filePat.isTrivialMatchAll())`?  I think that will not speed up `KEEP (*crtbegin.o(.ctors))` as in GNU ld's internal linker scripts. The idea is, in computeInputSections, when testing sections. The sections of the same file are contiguous. So a single-element cache can speed up.
> 
> The description should be modified to mention this point.
@grimar, the null `InputFile` for `matchesFile()` isn't quite so straightforward because `isTrivialMatchAll()` only currently identifies `"*"` and not `"**"` (or any other sequence of `'*'`). But yes, something like you suggest could be done as another change if desired.

@MaskRay, the `isTrivialMatchAll()` is there to catch the common case of the file glob pattern being `"*"`. This was more important before I added `getNameForScript()` but it is still beneficial which is why I have left it in. I have updated the description regarding the speed up due to the caching of the glob pattern match.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87469/new/

https://reviews.llvm.org/D87469