inThirdPartyPath has a large overhead

RESOLVED FIXED in Firefox 64

Status

defect
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla65
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed, firefox65 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

8 months ago
About half the build time regression from enabling the clang plugin (see bug 1500274 comment 7) can be attributed to NonParamInsideFunctionDeclChecker and a perf profile from an complete build attributes a large amount of time spent in inThirdPartyPath.
Assignee

Comment 1

8 months ago
It's even possible the rest of the build time regression is essentially due to inThirdPartPath too, because there are code paths using it that don't involve the NonParamInsideFunctionDeclChecker. As a matter of fact, the overhead from using the clang-plugin on Windows is even larger than on Linux, and it appears there's a code path leading to inThirdPartyPath from the Windows-only LoadLibraryUsageChecker.
Summary: NonParamInsideFunctionDeclChecker has a large overhead → inThirdPartyPath has a large overhead
Assignee

Updated

8 months ago
Assignee: nobody → mh+mozilla
Assignee

Comment 2

8 months ago
Looking at the source locations inThirdPartyPath is called for when building a file I picked semi-randomly (Unified_cpp_memory_build0.o), I can see it's called a lot of times for a very few number of different files. And for each of them, it scans the whole MOZ_THIRD_PARTY_PATHS list, because in most cases, the paths do not belong to third party directories. For instance, for that specific file, there are 4073 calls to inThirdPartyPath for only 75 unique paths.
Assignee

Updated

8 months ago
Blocks: 1500344
Assignee

Comment 3

8 months ago
SourceLocation that are passed to inThirdPartyPath might be macro
expansion locations, for which SourceManager.getFilename returns the
path of the directory containing the source, rather than of the
expansion location.

Furthermore, the paths getFileName returns are not canonical, and can
contain e.g. `..`.
Assignee

Comment 4

8 months ago
The function is called a lot for the same paths and is rather costly, so
cache the results for each path.

Depends on D9758

Comment 5

8 months ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/20af1d9ed3cd
Properly handle paths in inThirdPartyPath. r=andi
https://hg.mozilla.org/integration/autoland/rev/1adc093c7842
Cache the results of inThirdPartyPath. r=andi

Comment 6

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20af1d9ed3cd
https://hg.mozilla.org/mozilla-central/rev/1adc093c7842
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee

Comment 8

8 months ago
Comment on attachment 9019960 [details]
Bug 1501903 - Properly handle paths in inThirdPartyPath.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1486554

User impact if declined: This is required for the second patch in this bug.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:
Attachment #9019960 - Flags: approval-mozilla-beta?
Assignee

Comment 9

8 months ago
Comment on attachment 9019961 [details]
Bug 1501903 - Cache the results of inThirdPartyPath.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1486554

User impact if declined: Bug 1486554 caused some serious build time regressions on Windows, caused by enabling the clang plugin. This doesn't affect the code generated by Firefox, but changes some static analysis checks.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String changes made/needed:
Attachment #9019961 - Flags: approval-mozilla-beta?
Assignee

Comment 10

8 months ago
(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 9019960 [details]
> Bug 1501903 - Properly handle paths in inThirdPartyPath.

The new form completely failed on me... it didn't ask most of those questions.
 
> [Beta/Release Uplift Approval Request]
> 
> Feature/Bug causing the regression: Bug 1486554
> 
> User impact if declined: This is required for the second patch in this bug.
> 
> Is this code covered by automated tests?: Yes
> 
> Has the fix been verified in Nightly?: Yes
> 
> Needs manual test from QE?:

No

> If yes, steps to reproduce: 
> 
> List of other uplifts needed: None
> 
> Risk to taking this patch:

Low
 
> Why is the change risky/not risky? (and alternatives if risky): 

This doesn't affect the code generated by Firefox, but changes some static analysis checks.

> String changes made/needed:

N/A
Assignee

Comment 11

8 months ago
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 9019961 [details]
> Bug 1501903 - Cache the results of inThirdPartyPath.
> 
> [Beta/Release Uplift Approval Request]
> 
> Feature/Bug causing the regression: Bug 1486554
> 
> User impact if declined: Bug 1486554 caused some serious build time
> regressions on Windows, caused by enabling the clang plugin. This doesn't
> affect the code generated by Firefox, but changes some static analysis
> checks.
> 
> Is this code covered by automated tests?: Yes
> 
> Has the fix been verified in Nightly?: Yes
> 
> Needs manual test from QE?:

No
 
> If yes, steps to reproduce: 
> 
> List of other uplifts needed: None
> 
> Risk to taking this patch:

Low
 
> Why is the change risky/not risky? (and alternatives if risky): 

This doesn't affect the code generated by Firefox, but changes some static analysis checks.
 
> String changes made/needed:

N/A
Comment on attachment 9019960 [details]
Bug 1501903 - Properly handle paths in inThirdPartyPath.

[Triage Comment]
Fixes a Windows build time regression when the clang static analysis checks were moved into the build jobs. Approved for 64.0b5.
Attachment #9019960 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9019961 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.