1316642 - PendingLookup::IsBinaryFile is ridiculously bloated

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Nathan Froyd [:froydnj]]

PendingLookup::IsBinaryFile contains a 200-line condition checking whether a filename we're going to download is a binary file.  This condition compiles to a 200KB (!) function on x86-64 Linux.  I have a sense that GCC is doing something incredibly weird (possibly having to do with making sure it runs destructors for the NS_LITERAL_STRINGs correctly), but we are also doing a very poor job on this; this function should be nothing more than:

  for (auto& ext : kBinaryFileExtensions) {
    if (StringEndsWith(fileName, nsDependentString(ext.str, ext.length))) {
      return true;
    }
  }
  return false;

or something similar.

Comment 1

[Nathan Froyd [:froydnj]]

PendingLookup::GetDownloadType has the same sort of issue, only not quite as severe.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

froyd, do you want to try to extract a testcase and file a bug against gcc? We can fix this one easily but the inefficiency that caused it might be hurting us in other parts of the codebase.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: convert giant logical OR expression in IsBinaryFile into a for loop

The giant logical OR expression was causing GCC on x86-64 to churn out a
200K function, which is completely unnecessary.  This formulation is not
maximally efficient in space or time, but it is a massive improvement
size-wise: IsBinaryFile + the table is about 2K on x86-64.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8809532 [details] [diff] [review]
convert giant logical OR expression in IsBinaryFile into a for loop

Review of attachment 8809532 [details] [diff] [review]:
-----------------------------------------------------------------

Does our compiler support still not allow std::array and range-based for loops?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4335472b7aa7
convert giant logical OR expression in IsBinaryFile into a for loop; r=gcp

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> froyd, do you want to try to extract a testcase and file a bug against gcc?
> We can fix this one easily but the inefficiency that caused it might be
> hurting us in other parts of the codebase.

Filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78323

I don't think this specific issue hurts us, but it does illustrate that NS_LITERAL_STRING is not quite as efficient as we might wish. :(

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4335472b7aa7