Closed Bug 1455892 Opened Last year Closed Last year

ipdl.py can be quite slow in no-op builds

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: cmanchester, Assigned: cmanchester)

References

Details

Attachments

(1 file)

Due to the same class of bug as bug 1455722 (ipdl.py will not touch output files if its output hasn't changed), ipdl.py can be quite slow in a no-op build (~15 seconds for export, should be 0).
Sorry, typo in the bug summary (not comment 0).
Summary: xpidl.py can be quite slow in no-op builds → ipdl.py can be quite slow in no-op builds
I hacked up a quick patch for this... will request feedback shortly.
Assignee: nobody → cmanchester
Comment on attachment 8972203 [details]
Bug 1455892 - Use a tracking stub file rather than a phony target to speed up ipdl in incremental builds.

Always running the "ipdl" target was almost certainly papering over some under-specified dependencies for the target, so this is probably going to result in incorrect builds some of the time (some mozbuild python files imported by ipdl.py don't end up as dependencies for instance). So the question is whether this is better or worse than the ~12s of running the target during every incremental build that happens in a tree after an ipdl input has been touched (not just the incremental build after touching an ipdl input, but every build in that tree thereafter as well).

Requesting feedback from Nathan as our ipc and build peer.
Attachment #8972203 - Flags: feedback?(nfroyd)
Comment on attachment 8972203 [details]
Bug 1455892 - Use a tracking stub file rather than a phony target to speed up ipdl in incremental builds.

This is a reasonable solution.  But ipdl.py already has some mtime checking and whatnot; why isn't that doing the right thing here?  Is that fixable, or not?
Attachment #8972203 - Flags: feedback?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8972203 [details]
> Bug 1455892 - Use a tracking stub file rather than a phony target to speed
> up ipdl in incremental builds.
> 
> This is a reasonable solution.  But ipdl.py already has some mtime checking
> and whatnot; why isn't that doing the right thing here?  Is that fixable, or
> not?

The issue is that our "writeifmodified" checks undermine our "latestipdlmod" checks: in any built tree if you touch a file used as input to the "latestipdlmod" check ipdl parsing/typechecking/codegen will take place, but "writeifmodified" will mean that almost certainly not all output files will be touched in this build, so every "latestipdlmod" check in subsequent builds will see that unmodified output as older than the input that caused that build, and parsing/typechecking/codegen will happen again.

The "writeifmodified" check should stay, and we shouldn't start touching every output file because this is probably saving incremental build time for downstream compilation, but if we take this patch the "latestipdlmod" check should probably go away.
Duplicate of this bug: 907394
Comment on attachment 8972203 [details]
Bug 1455892 - Use a tracking stub file rather than a phony target to speed up ipdl in incremental builds.

https://reviewboard.mozilla.org/r/240864/#review247296

Sure, let's do this.

::: ipc/ipdl/Makefile.in:15
(Diff revision 3)
> +ipdl_py_deps := \
> +    $(wildcard $(srcdir)/*.py) \
> +    $(wildcard $(srcdir)/ipdl/*.py) \
> +    $(wildcard $(srcdir)/ipdl/cxx/*.py) \

This hardcoded list means we miss important things like the `ply` dependency in `ipdl/parser.py`.  I guess we have decided we're OK with botching builds in exchange for most builds becoming somewhat faster?  Especially because `ply` is unlikely to be updated frequently...

I see that we have a `PLY_INCLUDE` down below, perhaps we could still extract the `.py` dependencies from that?

::: ipc/ipdl/ipdl.py:7
(Diff revision 3)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import optparse, os, re, sys
>  from cStringIO import StringIO
>  from mozbuild.pythonutil import iter_modules_in_path

We can probably remove this import now?
Attachment #8972203 - Flags: review?(nfroyd) → review+
Comment on attachment 8972203 [details]
Bug 1455892 - Use a tracking stub file rather than a phony target to speed up ipdl in incremental builds.

https://reviewboard.mozilla.org/r/240864/#review247296

> This hardcoded list means we miss important things like the `ply` dependency in `ipdl/parser.py`.  I guess we have decided we're OK with botching builds in exchange for most builds becoming somewhat faster?  Especially because `ply` is unlikely to be updated frequently...
> 
> I see that we have a `PLY_INCLUDE` down below, perhaps we could still extract the `.py` dependencies from that?

Yes good suggestion, I'll add that. From looking at the python code under `ipdl`, `ply` is the only non standard library import happening there, so this should be relatively safe.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23400e395408
Use a tracking stub file rather than a phony target to speed up ipdl in incremental builds. r=froydnj
Backed out changeset 23400e395408 (bug 1455892) for linux build bustages on ContentBridgeParent.h

Back out link: https://hg.mozilla.org/integration/autoland/rev/b63df024b6129a1246884b7a251e02ea9f4b45a2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=23400e395408c6816d3afc0d4c192013b4cd8e69

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=176829284&repo=autoland&lineNumber=46192

task 2018-05-03T20:09:33.992Z] 20:09:33     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o collationiterator.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DU_I18N_IMPLEMENTATION -DUCONFIG_NO_TRANSLITERATION -DUCONFIG_NO_REGULAR_EXPRESSIONS -DUCONFIG_NO_LEGACY_CONVERSION -DU_USING_ICU_NAMESPACE=0 -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DU_CHARSET_IS_UTF8 -DU_HAVE_NL_LANGINFO_CODESET=0 -I/builds/worker/workspace/build/src/config/external/icu/i18n -I/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n -I/builds/worker/workspace/build/src/intl/icu/source/common -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O3 -fno-omit-frame-pointer -frtti -fprofile-use -fprofile-correction -Wcoverage-mismatch -MD -MP -MF .deps/collationiterator.o.pp   /builds/worker/workspace/build/src/intl/icu/source/i18n/collationiterator.cpp
[task 2018-05-03T20:09:33.994Z] 20:09:33     INFO -  make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-03T20:09:34.002Z] 20:09:34     INFO -  make[5]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-03T20:09:34.003Z] 20:09:34     INFO -  make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-03T20:09:34.023Z] 20:09:34     INFO -  make[5]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-03T20:09:34.023Z] 20:09:34     INFO -  make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/i18n'
[task 2018-05-03T20:09:34.064Z] 20:09:34     INFO -  make[5]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker'
[task 2018-05-03T20:09:34.071Z] 20:09:34     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o SandboxBrokerPolicyFactory.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/security/sandbox/linux/broker -I/builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker -I/builds/worker/workspace/build/src/security/sandbox/linux -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/security/sandbox/chromium -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O3 -fno-omit-frame-pointer -Werror -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fprofile-use -fprofile-correction -Wcoverage-mismatch -MD -MP -MF .deps/SandboxBrokerPolicyFactory.o.pp   /builds/worker/workspace/build/src/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
[task 2018-05-03T20:09:34.072Z] 20:09:34     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/ContentChild.h:12:0,
[task 2018-05-03T20:09:34.072Z] 20:09:34     INFO -                   from /builds/worker/workspace/build/src/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:18:
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/ContentBridgeParent.h:10:46: fatal error: mozilla/dom/PContentBridgeParent.h: No such file or directory
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -   #include "mozilla/dom/PContentBridgeParent.h"
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -                                                ^
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -  compilation terminated.
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1024: recipe for target 'SandboxBrokerPolicyFactory.o' failed
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -  make[5]: *** [SandboxBrokerPolicyFactory.o] Error 1
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -  make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/sandbox/linux/broker'
[task 2018-05-03T20:09:34.073Z] 20:09:34     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'security/sandbox/linux/broker/target' failed
[task 2018-05-03T20:09:34.074Z] 20:09:34     INFO -  make[4]: *** [security/sandbox/linux/broker/target] Error 2
[task 2018-05-03T20:09:34.074Z] 20:09:34     INFO -  make[4]: *** Waiting for unfinished jobs....
Flags: needinfo?(cmanchester)
The stub file needs to be added to GARBAGE so we re-generate the sources in the profiling stage. Updated patch coming up.
Flags: needinfo?(cmanchester)
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ceda3f51c77
Use a tracking stub file rather than a phony target to speed up ipdl in incremental builds. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/4ceda3f51c77
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.