362392 - mddepend.pl stat() optimizations

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Kenneth Herron]

A few things can be done to reduce the number of |stat()| calls performed by mddepend.pl:

1) The loop which iterates over dependencies (see lines 91-102 in the sample URL) could be broken into two passes. The first pass only checks the %alldeps cache for each dependency, and saves each dep that isn't in the cache. The second pass only runs if necessary, and actually stats the deps that were skipped during the first pass.

2) The cache-checking code doesn't distinguish between deps that haven't been stat'ed and aren't in the cache, and deps that don't exist and are in the cache with a time of undef. As a result, a dep that doesn't exist is rechecked for each target that depends on it.

3) %alldeps is initialize incorrectly. The "{}" on line 54 is a hash reference, so %alldeps ends up with a bogus target named "HASH(0xsomething)". This causes an extra pass through the target loop. Line 54 should probably just be "my %alldeps;"

In my simple tests, these changes reduce the number of stat calls by roughly 25%. Most of the benefit is from #1. If nobody else jumps on this, I'll submit a patch this weekend.

Comment 1

[jag (Peter Annema)]

Attachment: Something along these lines?

This follows suggestion 1: try to find a reason to force a rebuild for the object (which lets us move on to the next object) in the dependencies' mtime cache first. If not, stat() the untested dependencies.

Fixes point 2 by using -1 as the "stat returned nothing, assume file doesn't exist any longer" code.

Addresses point 3.

Use strict, add |my| in a few places, parse the files slightly differently to reduce the complexity of the parsing loop, fix a few bugs in debug printing of added dependencies, add more debug output (very handy, it let me find bug 416571).

I've made a functional change (which I could split off to a new bug): for missing dependencies I now force the object to be rebuilt, so that for depend builds if someone e.g. removed a .h from the build system without updating the .cpp to no longer include it we will actually get a build failure.

Comment 2

[jag (Peter Annema)]

Silly me, I'm stuffing duplicates into @objs and then stat()ing them all. Makes things slower, not faster ;-)

Comment 3

[jag (Peter Annema)]

Erh, @not_in_cache, that is.

Comment 4

[jag (Peter Annema)]

Attachment: Fixes thinko in previous patch

Comment 5

[[e-mail]]

It seems to me that after you've marked all the deps uncached the first time around, you'll never add them to the list of uncached deps for later files.

Comment 6

[jag (Peter Annema)]

Attachment: Use hash for the second phase, also make sure to mark failed stats with -1

Good catch! I chose to make not_in_cache a hash instead of walking the rest of the array to clear -2 entries from %modtimes. I'd also taken out the || -1 from the second inner loop ('coz I can just check against not defined, right? ;-)), forgetting that we wouldn't mind having that knowledge for the next object in the first inner loop.

Comment 7

[[e-mail]]

Comment on attachment 302498 [details] [diff] [review]
Use hash for the second phase, also make sure to mark failed stats with -1

Note: failing on missing files causes problems with .gqi files because their dependencies aren't correctly generated on Windows (msys perl appears to require forward slashes on all paths unless they start with a drive letter!)

Also I think your debug flags are odd: DEBUG = 1 only prints the junk.

Comment 8

[jag (Peter Annema)]

DEBUG => 2 will print even more junk ;-)

So I was thinking of just printing the reason for "why does this need a rebuild?" and the ".all.pp updated" messages at level 1, and add everything else at level 2. I just noticed that the "STAT" lines are in level 1, I'll fix that before I check in.

Comment 9

[jag (Peter Annema)]

Comment on attachment 302498 [details] [diff] [review]
Use hash for the second phase, also make sure to mark failed stats with -1

bsmedberg gave mao+. The debug stuff works as designed, but the logic is a bit confusing. If I can find a way to simplify it I'll file a follow-up bug.

Comment 10

[jag (Peter Annema)]

Waiting to check-in until bug 416843 is fixed.

Comment 11

[jag (Peter Annema)]

Checking in build/unix/mddepend.pl;
/cvsroot/mozilla/build/unix/mddepend.pl,v  <--  mddepend.pl
new revision: 1.11; previous revision: 1.10
done