Closed Bug 1316642 Opened 3 years ago Closed 3 years ago

PendingLookup::IsBinaryFile is ridiculously bloated

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

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.
PendingLookup::GetDownloadType has the same sort of issue, only not quite as severe.
Blocks: 1291472
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.
Flags: needinfo?(nfroyd)
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.
Attachment #8809532 - Flags: review?(gpascutto)
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?
Attachment #8809532 - Flags: review?(gpascutto) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4335472b7aa7
convert giant logical OR expression in IsBinaryFile into a for loop; r=gcp
(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. :(
Flags: needinfo?(nfroyd)
Assignee: nobody → nfroyd
https://hg.mozilla.org/mozilla-central/rev/4335472b7aa7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.