1501903 - inThirdPartyPath has a large overhead

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

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.

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1501903 - Properly handle paths in inThirdPartyPath.

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. `..`.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1501903 - Cache the results of inThirdPartyPath.

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

[Pulsebot]

Pushed by [e-mail]:
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

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/20af1d9ed3cd
https://hg.mozilla.org/mozilla-central/rev/1adc093c7842

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/20af1d9ed3cd
https://hg.mozilla.org/mozilla-central/rev/1adc093c7842

Comment 8

[Mike Hommey [:glandium]]

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:

Comment 9

[Mike Hommey [:glandium]]

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:

Comment 10

[Mike Hommey [:glandium]]

(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

Comment 11

[Mike Hommey [:glandium]]

(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 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a186639fc5c5
https://hg.mozilla.org/releases/mozilla-beta/rev/2058a60f13a7