Closed Bug 995762 Opened 9 years ago Closed 9 years ago processing could take as much as 15 minutes!


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: ehsan.akhgari, Assigned: mshal)



(4 files, 2 obsolete files)

I just rebased on top of master and did a ./mach build, and here's what happened:

 0:17.57 checking for LC_MESSAGES... (cached) yes
 0:17.57 checking for localeconv... (cached) yes
 0:17.64 checking for __cxa_demangle... (cached) yes
 0:17.65 checking for -pipe support... yes
 0:17.66 checking whether C compiler supports -fprofile-generate... yes
 0:17.68 checking for correct overload resolution with const and templates... no
 0:17.69 checking for tm_zone tm_gmtoff in struct tm... (cached) yes
 0:17.74 checking what kind of list files are supported by the linker... filelist
 0:17.76 checking for posix_fadvise... (cached) no
 0:17.77 checking for posix_fallocate... (cached) no
 0:17.80 checking for setlocale... (cached) yes
 0:17.80 checking for localeconv... (cached) yes
 0:17.81 creating ./config.status
 0:18.02 Reticulating splines...
15:38.34 Finished reading 2541 files in 1.91s
15:38.34 Processed into 6927 build config descriptors in 4.01s
15:38.34 Backend executed in 914.20s
15:38.34 2573 total backend files; 3 created; 40 updated; 2530 unchanged; 0 deleted; 238 -> 1025 Makefile
15:38.34 Total wall time: 920.32s; CPU time: 919.51s; Efficiency: 100%; Untracked: 0.20s
15:38.76 From dist/public: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
15:38.77 From dist/sdk: Kept 0 existing; Added/updated 0; Removed 26 files and 4 directories.
15:38.82 From dist/private: Kept 0 existing; Added/updated 0; Removed 155 files and 2 directories.
15:39.45 From dist/bin: Kept 85 existing; Added/updated 0; Removed 3432 files and 356 directories.
15:39.49 From dist/idl: Kept 1138 existing; Added/updated 1; Removed 0 files and 0 directories.
15:39.66 From dist/include: Kept 3979 existing; Added/updated 4; Removed 192 files and 6 directories.
15:41.00 From _tests: Kept 17373 existing; Added/updated 13; Removed 764 files and 110 directories.
15:41.24 cd include; /Applications/ private_export
15:41.24 Creating /Users/ehsan/moz/src/obj-ff-dbg.noindex/dist/private/nss
15:41.25 cd mangle; /Applications/ private_export
15:41.27 Creating /Users/ehsan/moz/src/obj-ff-dbg.noindex/dist/private/dbm

Not sure at all what happened there.  Previously I was on, and now I am on
Attached file Full build log
This is all of my local modifications
To make things more interesting, after a clobber I could not reproduce.  My tree was fully built before the rebase of course.
Attached image profile-bad.png
I can reproduce this by switching between git commits d506740 and efc5497c, and just running './config.status' in between. froydnj pointed out in IRC that it appears to be related to file diffing. Attached is a profile graph from a bad run (that takes ~3 minutes instead of the usual ~9 seconds).

The slow runtime goes away if I force self._capture_diff to False in FileAvoidWrite(). I'm still looking at why exactly the diffing is so slow - just thought I'd mention it in case gps would know off-hand :)
At least in the particular case of the two commits I tested, the slow runtime is all because of diffing all-tests.json, which is ~15MB.
It seems the problem is a severe performance limitation of difflib, and I'm not sure how easy that will be to fix. In the meantime we can avoid evaluating the diff generator unless --diff is specified. The '\n'.join() was causing the immediate evaluation even if --diff wasn't used, so this patch moves the join to where the diff is displayed.

Note that fh.diff is also used in BackendMakeFile:diff(), but I don't know if that's actually used.
Attachment #8406280 - Flags: review?(gps)
Comment on attachment 8406280 [details] [diff] [review]

Review of attachment 8406280 [details] [diff] [review]:

Generators FTW! Great patch.
Attachment #8406280 - Flags: review?(gps) → review+
Unfortunately my patch breaks 'make check' due to 'self.assertIsInstance(faw.diff, unicode)' lines in, since the generator object doesn't count as unicode. Should I just delete those assertIsInstance lines? Or is there a better way to fix that?
Flags: needinfo?(gps)
Assuming that removing those lines is ok, here's the new patch. It passes try this time:

If you want me to fix the tests some other way just let me know!
Attachment #8406280 - Attachment is obsolete: true
Attachment #8406853 - Flags: review?(gps)
Flags: needinfo?(gps)
Assignee: nobody → mshal
Comment on attachment 8406853 [details] [diff] [review]

Review of attachment 8406853 [details] [diff] [review]:

::: python/mozbuild/mozbuild/test/
@@ -132,2 @@
>              self.assertIn('-old', faw.diff)
>              self.assertIn('+new', faw.diff)

Oh, I'm surprised this still works!

I would rewrite this as:

diff = '\n'.join(faw.diff)
self.assertIn('-old', diff)
self.assertIn('+new', diff)

@@ -141,5 @@
>              faw = FileAvoidWrite(path, capture_diff=True)
>              faw.write('new')
>              faw.close()
> -            self.assertIsInstance(faw.diff, unicode)

Ditto. Expand the diff inline.
Attachment #8406853 - Flags: review?(gps) → review+
Updated with review comments, r+ carried forward.
Attachment #8406853 - Attachment is obsolete: true
Attachment #8408244 - Flags: review+
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.