Closed Bug 515792 Opened 15 years ago Closed 14 years ago

build system runs identical 'hg identify' command too many times

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dbaron, Assigned: Gijs)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

'hg identify' can be quite slow:
 * because it does a full-tree diff
 * for other reasons, unclear to me, when a tree has a large number of heads
and we call it a significant number of times.  (I have a tree with a large number of heads in which it can take a significant part of a minute.)

We call an identical hg identify command from three different makefiles:
http://mxr.mozilla.org/mozilla-central/search?string=hg%20identi
for every phase of the build (export, libs, etc.(?)).

We should probably just call it once, either in configure (and put the output in a variable that gets AC_SUBSTed into autoconf.mk), or in the export phase of config/Makefile.in or something similar (and put the output in a file that can be cat-ed elsewhere).
I think we could also change the command slightly so it doesn't try to figure out if you have local changes. Something like:
hg identify -ir. ('.' is "the parent changeset of the working directory")
You can also get that from:
hg parent --template="{node|short}\n"
Comment #2 makes this sound like the easiest build-speed win available!
10 minute build times, here we come!
Something like this?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #409345 - Flags: review?(ted.mielczarek)
You could also AC_DEFINE it and then remove the manual addition to DEFINES in the two makefiles, I think.
Is running configure each time we update the tree really going to speed things up?
Attached patch Patch (obsolete) — Splinter Review
So the previous patch wouldn't have worked, I don't think (I mean, it built, but MOZ_SOURCE_STAMP was actually empty). This, on the other hand, does work (tested with about:buildconfig, which uses the variable). It adds MOZ_SOURCE_STAMP as an export in the toplevel makefile, and then in the sub-makefiles only assigns to that variable if it's not already defined. This ensures that the value is up-to-date even if make is only ran in subdirectories (thanks, Axel!).
Attachment #409345 - Attachment is obsolete: true
Attachment #410760 - Flags: review?(ted.mielczarek)
Attachment #409345 - Flags: review?(ted.mielczarek)
Comment on attachment 410760 [details] [diff] [review]
Patch

+MOZ_SOURCE_STAMP = $(shell cd $(srcdir) && hg parent --template="{node|short}\n" 2>/dev/null)

Is this going to have the same problem that bug 517417 fixed, in that hg will look for a repo above the current directory?
Arne: could you test if the command in this patch (quoted in my previous comment) exhibits the same problem as bug 517417? I don't want to regress that for you.
Ted: Thank you for asking! 

It doesn't exibit the same problem, but it would lead to false information for your buildsystem (since it still finds the higher up hg repo). 

To make sure that the command bails out when it isn't in the top level directory of the repo, you can use 

hg parent --repository . --template="{node|short}\n"

But the cleanest way would probably be to use 

MOZ_SOURCE_STAMP ?= $(shell hg --repository $(topsrcdir) parent --template="{node|short}\n" 2>/dev/null)

This checks explicitely if $(topsrcdir) is a hg repository *root* and if yes it returns its parent. A shorthand would be "hg -R" instead of "hg --repository".
I think we might not have been using -R because we are trying to support some ancient version of hg (0.9.4?) that comes with Scratchbox, which our mobile developers use.
Could you test if -R works for them? 

For me hg 0.9.4 uses -R - I just tested it with 0.9, too.
(In reply to comment #13)
> Could you test if -R works for them? 
> 
> For me hg 0.9.4 uses -R - I just tested it with 0.9, too.

This works for me in scratchbox, too:

[sbox-CHINOOK-ARMEL-2007: ~/devmoz/mozilla-central] > hg -R . parent --template="{node|short}\n" 2>/dev/null
001e14d17a30

Updated the patch using this approach instead.
Attachment #410760 - Attachment is obsolete: true
Attachment #413296 - Flags: review?(ted.mielczarek)
Attachment #410760 - Flags: review?(ted.mielczarek)
Ok, I wasn't sure, I just remembered that that code went through several revisions, and we did have problems with the old hg in scratchbox. Thanks for testing!
Thank you for taking care!
Comment on attachment 413296 [details] [diff] [review]
Patch using -R flag

Looks good, thanks!
Attachment #413296 - Flags: review?(ted.mielczarek) → review+
Depends on: 517417
Flags: in-testsuite-
http://hg.mozilla.org/mozilla-central/rev/fec4a5e4b36e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Fwiw, should
{
/toolkit/crashreporter/tools/symbolstore.py
    * line 276 -- rev = os.popen('hg identify -i "%s"' % srcdir, "r").readlines()[0].rstrip()
}
be updated too?
Flags: wanted1.9.2?
Target Milestone: --- → mozilla1.9.3a1
Blocks: 536136
No longer blocks: 524349
No, not worth it.
And I don't see any reason we'd take this on 1.9.2.
Flags: wanted1.9.2? → wanted1.9.2-
Depends on: 536173
Depends on: 553877
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.