Prototype for de-recursifying js/src's build system

RESOLVED WORKSFORME

Status

()

Core
Build Config
RESOLVED WORKSFORME
6 years ago
7 months ago

People

(Reporter: Sander Mathijs van Veen, Assigned: Sander Mathijs van Veen)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
During the last few days, I've been working on a prototype build system. The
main purpose is faster (re)builds. This prototype is currently only targeted at
the JavaScript engine (js/src), but the concept can be applied to all other
components as well. The current state of this build system is that it
successfully builds the JavaScript shell and libmozjs for a Linux 64 bit
architecture.


Design goals of this prototype:
===============================

 1. Faster (re)builds (improved parallelism, just one Make process).
 2. Easy to maintain the build system. Root Makefile (in this prototype called
    "js/src/fast.mk") includes all Rules.mk files from the subdirs.
 3. Object files are added on a per-directory level.
 4. Specify CFLAGS for object files per-directory level.
 5. Deleting "obj-linux" will remove all generated files (e.g. object files and
    generated header files). Thus, there are no more object files intermixed with
    the source files.


Overview of the build system:
===============================

Per directory Rules.mk files (including js/src itself):

|-- Rules.mk
|-- assembler
|   |-- Rules.mk
|   |-- assembler
|   |   |-- Rules.mk
|   |-- jit
|   |   `-- Rules.mk
|   `-- wtf
|       |-- Rules.mk
|-- config
|   |-- Rules.mk
|-- editline
|   |-- Rules.mk
|-- frontend
|   |-- Rules.mk
|-- jsapi-tests
|       |-- Rules.mk
|-- methodjit
|   |-- Rules.mk
[...]
|-- vm
|   |-- Rules.mk
|-- yarr
|   |-- Rules.mk

The rest of the build system is stored in:
 - js/src/fast.mk (includes all Rules.mk files from the subdirs).
 - js/src/build/common.mk (common logic to build object files from c++ files).

The main Makefile is currently called "fast.mk", since I wanted to
test/benchmark both build systems. This allows having both build systems in a
single tree and makes it easier for someone else to test the non-recursive
build system (since it doesn't modify the existing build system).

I chose "Rules.mk", since there is a file "config/rules.mk" already. The file
"js/src/build/common.mk" is used in almost every Rules.mk, and it eliminates
redundant lines in Rules.mk.

As mentioned earlier, all object files and generated files are stored in
"js/src/obj-linux". Currently, the object files are intermixed with the source
files. IMHO, I think it is better to separate generated content from editable
source files.


Benchmarks (http://pastebin.mozilla.org/1307752):
===============================

Machine: Gentoo amd64, i7, 8 GB ram; build: -Os.

                    | Original | Non-recursive |
Complete rebuild    | 30 sec   | 26 sec        |
"touch jsbool.h"    | 25 sec   | 22 sec        |
"touch jsreflect.h" | 6.7 sec  | 4.3 sec       |


Usage of non-recursive build system:
===============================

As mentioned above, this prototype is only tested on a 64 bit Linux box. After
applying the attached patch, the following instruction should build the
JavaScript engine using the non-recursive build system:

cd js/src
./configure
make -ffast.mk -j12

Similar to the Kernel's build system, I also added a more silent mode:

make -ffast.mk -j12 SILENT=1

This is similar to the current build system's output of "make -s", but does
also display the progress of linking libmozjs and other binaries. Currently,
SILENT is an environment variable, but this could be added to the
configure-script to enable it by default (and make it toggle-able from the
command line using SILENT=0).


TODO list of this prototype (IMHO, ordered on most useful to do):
===============================
 1. remove redundant CFLAGS from all Rules.mk.
 2. better integration with the files generated from the existing ./configure
    script (e.g. integrate more variables from build/config.mk in fast.mk and
    in all Rules.mk files).
 3. support linux 32 bit architecture.
 4. start working on support for other platforms and architectures.


In order to keep the patch and still be able to build the whole source tree, I
noticed that check-sync-dirs reports that the build-dirs mismatch. To fix this,
use: 

ln -s {../js/src/,}build/common.mk
ln -s {../js/src/,}config/Rules.mk

Later on, I hope that this prototype concept can be used to re-engineer
time-consuming parts of the Firefox source tree. I'm looking forward to
feedback about improvement of this build system and feedback of testers.
(Assignee)

Comment 1

6 years ago
Created attachment 554786 [details] [diff] [review]
Non-recursive build system prototype for js/src (64 bit linux only)

Comment 2

6 years ago
Sounds like Joey would be interested in this.

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Related: bug 623617
See Also: → bug 623617
(Assignee)

Comment 4

6 years ago
Created attachment 555009 [details] [diff] [review]
Non-recursive build system prototype for js/src (32 and 64 bit linux only)

Compared to the first patch, this patch contains the following improvements:

 1. remove redundant CFLAGS from all Rules.mk files.
 2. better integration with the files generated from the existing ./configure
    script (e.g. integrate more variables from build/config.mk in fast.mk and
    in all Rules.mk files).
 3. support linux 32 bit architecture.

So, the last item of the TODO list remains:

 4. start working on support for other platforms and architectures.

Currently, I'm requesting Level 1 access to the try server, which will help testing the different platforms and architecture.

The refactoring of this patch allows using a general template for Rules.mk file:

  include build/header.mk
  
  # Rules for <Component>
  
  OBJ_$(d) := \
  	$(b)/<file0>.o \
  	$(b)/<file1>.o \
  	$(b)/<file2>.o \
  
  include build/common.mk
  
  include build/footer.mk

The variables $(d) and $(b) are set in build/header.mk and there value is respectively the relative path to the source directory and the build directory. Besides the OBJ_$(d) variable, it is also common to add libmozjs.so dependencies:

  OBJ_LIBMOZJS += $(OBJ_$(d))

and directory-specific CFLAGS:

  CFLAGS_$(d) += -I. -I./assembler -I./nanojit -I./tracejit

Currently, there is no CXXFLAGS_$(d). The variable CFLAGS_$(d) is used for both CXX and CC invocation.
Attachment #554786 - Attachment is obsolete: true
(In reply to Sander Mathijs van Veen from comment #4)
> Currently, I'm requesting Level 1 access to the try server, which will help
> testing the different platforms and architecture.

In the meantime if you wish me to send things to try for you, I'm more than happy to do so. Either ask here, ping me via email or ask me (edmorley) on #pymake/#developers.
(Assignee)

Comment 6

6 years ago
Created attachment 558827 [details] [diff] [review]
Non-recursive build system prototype for js/src (32 and 64 bit linux only)

Updated the patch to support m-c's current tip.

See also the design goals mentioned in the comments above. I tested the patch on 32 and 64 bit Gentoo Linux:

cd js/src
./configure
make -ffast.mk -j12 SILENT=1 THIN_ARCHIVE=1

Note: both environment variables should be moved into the configure script. This patch does not affect the original build system.

Results:

-----------------------------------------
non-recursive build system

smvv@multivac ~/moz/mozilla-central/js/src $ make clean -s; rm -rf dist; ccache -C; time make -ffast.mk -sj12 THIN_ARCHIVE=1

real    0m40.928s
user    4m37.840s
sys     0m17.494s

smvv@multivac ~/moz/mozilla-central/js/src $ touch jsreflect.h
smvv@multivac ~/moz/mozilla-central/js/src $ time make -ffast.mk -sj12 THIN_ARCHIVE=1

real    0m2.884s
user    0m4.626s
sys     0m0.775s

-----------------------------------------
original build system

smvv@multivac ~/moz/mozilla-central/js/src $ make clean -s; rm -rf dist; ccache -C; time make -sj12

real    0m45.960s
user    4m34.661s
sys     0m18.422s

smvv@multivac ~/moz/mozilla-central/js/src $ touch jsreflect.h
smvv@multivac ~/moz/mozilla-central/js/src $ time make -sj12
jsreflect.cpp
js.cpp

real    0m6.470s
user    0m4.477s
sys     0m1.335s

-----------------------------------------

The THIN_ARCHIVE environment variable is a minor optimization for libjs_static.a. It adds the "T" flag to "ar"; instead of copying all object files into the archive, it creates a symbol index and references to the object files. See also this part of the ar man page:

       GNU ar can optionally create a thin archive, which contains a symbol index and
       references to the original copies of the member files of the archives.  Such an
       archive is useful for building libraries for use within a local build, where the
       relocatable objects are expected to remain available, and copying the contents of
       each object would only waste time and space.  Thin archives are also flattened, so
       that adding one or more archives to a thin archive will add the elements of the
       nested archive individually.  The paths to the elements of the archive are stored
       relative to the archive itself.

It saves approx. 1 second per rebuild/incremental build. What are the disadvantages of using thin archives?

-----------------------------------------

In order to analysis the build system, I created a simple script and web app to visualize the build system's behavior. The analysis of the non-recursive build system can be found at: http://smvv.kompiler.org/bsa/
Clicking on the bars shows the execve() system calls in the top window. If there are sub processes, the bar will expand and show the sub processes. The analysis is a waterfall graph and shows clearly that the non-recursive build system is good at parallelism. On almost every interval, 12 jobs are running in parallel. At the end of the graph, the linking is done and shows underutilisation of the remaining cores. I think that using the gold linker as well, the build times will improve even further.

Sidenote: The script uses strace and I think that logging each execve/vfork etc. system call has a negative impact on the timing results. In the waterfall graph, the top Make process' duration is 50 seconds, while without strace, the non-recursive build system needs only 40 seconds to complete its task. This is work in progress.
Assignee: nobody → sandervv
Attachment #555009 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #558827 - Flags: feedback?(khuey)
Note that we already have our own home-grown static library replacement, implemented in bug 584474. We generate $(LIBRARY).desc files that simply contain lists of object file names, and unwrap them before passing them to the linker.

(js_static.a is still being generated because of the "DIST_INSTALL = 1" in js/src/Makefile.in, we can probably fix that.)
I have a bunch of comments here, but I haven't gotten through the whole thing, so those probably won't get posted till tomorrow.  Sorry about that :-/
Comment on attachment 558827 [details] [diff] [review]
Non-recursive build system prototype for js/src (32 and 64 bit linux only)

Review of attachment 558827 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I took so long :-/

Looks pretty good, some general comments:

1) To land it we will of course need to replace all the hardcoded stuff with stuff that grabs the various variables directly from autoconf.mk.
2) Lets try to avoid target specific variables.  Pymake has different propagation behavior there.
3) The dependency logic probably needs to be more complex.  I remember there being cases that the simple include case doesn't catch (which escape me at the moment).

I think we can land this in the tree if we can get to a place where it works correctly on one platform (presumably Linux), honoring stuff from configure/etc, and it's designed in such a way that doesn't box us in in the future (e.g. it's not obviously incompatible with windows/pymake/etc)

::: js/src/Rules.mk
@@ +2,5 @@
> +
> +# Rules for JS engine
> +
> +LIB_MOZJS    := $(BUILD_DIR)/libmozjs.so
> +AR_JS_STATIC := $(BUILD_DIR)/libjs_static.a

Obviously these values are only good for Unix.  We'll want to grab the right stuff out of autoconf.mk.

@@ +16,5 @@
> +	$(b)/include/jsautooplen.h \
> +	$(b)/include/jsautokw.h \
> +	$(b)/include/jsautocfg.h \
> +
> +OBJ_LIBMOZJS += \

I'd rather list cpp files, than object files (mostly because the extension doesn't change across platforms).

@@ +81,5 @@
> +include build/common.mk
> +
> +$(OBJ_$(d)): COMPILE_CXXFLAGS += -I$(BUILD_DIR) -I./assembler -I./nanojit \
> +	-I./tracejit -DEXPORT_JS_API -DUSE_SYSTEM_MALLOC=1 -DMOZILLA_CLIENT \
> +	-DENABLE_ASSEMBLER=1 -DENABLE_JIT=1

I'd try to avoid target specific variables like this if at all possible.  Pymake doesn't propagate those down (in other words, COMPILE_CXXFLAGS won't be set for any prerequisites for $(OBJ_$(d))) because the behavior is racy.

@@ +123,5 @@
> +system_wrappers_js: $(SYSTEM_WRAPPERS_JS)/unix.h | $(b)/system_wrappers_js
> +
> +$(SYSTEM_WRAPPERS_JS)/%.h: $(d)/config/system-headers $(d)/config/make-system-wrappers.pl
> +	$(PYTHON) ./config/Preprocessor.py -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DHAVE_64BIT_OS=1 -DD_INO=d_ino -DJS_CPU_X64=1 -DJS_PUNBOX64=1 -DJS_METHODJIT=1 -DJS_MONOIC=1 -DJS_POLYIC=1 -DJS_POLYIC_TYPED_ARRAY=1 -DFEATURE_NANOJIT=1 -DJS_TRACER=1 -DAVMPLUS_AMD64=1 -DAVMPLUS_64BIT=1 -DAVMPLUS_UNIX=1 -DAVMPLUS_LINUX=1 -DSTDC_HEADERS=1 -DHAVE_SSIZE_T=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DJS_HAVE_STDINT_H=1 -DJS_BYTES_PER_WORD=8 -DJS_BITS_PER_WORD_LOG2=6 -DJS_ALIGN_OF_POINTER=8 -DJS_BYTES_PER_DOUBLE=8 -DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UNAME_DOMAINNAME_FIELD=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_ATTRIBUTE=1 -DHAVE_DIRENT_H=1 -DHAVE_GETOPT_H=1 -DHAVE_SYS_BITYPES_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_GNU_LIBC_VERSION_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_SYS_VFS_H=1 -DHAVE_SYS_MOUNT_H=1 -DHAVE_SYS_QUOTA_H=1 -DHAVE_LINUX_QUOTA_H=1 -DHAVE_MMINTRIN_H=1 -DNEW_H=\<new\> -DHAVE_SYS_CDEFS_H=1 -DHAVE_DLOPEN=1 -DHAVE_DLADDR=1 -D_REENTRANT=1 -DHAVE_FCHMOD=1 -DHAVE_FLOCKFILE=1 -DHAVE_GETC_UNLOCKED=1 -DHAVE_GETPAGESIZE=1 -DHAVE_LCHOWN=1 -DHAVE_LOCALTIME_R=1 -DHAVE_LSTAT64=1 -DHAVE_MEMMOVE=1 -DHAVE_RANDOM=1 -DHAVE_SBRK=1 -DHAVE_SNPRINTF=1 -DHAVE_STAT64=1 -DHAVE_STATVFS=1 -DHAVE_STATVFS64=1 -DHAVE_STRERROR=1 -DHAVE_STRTOK_R=1 -DHAVE_TRUNCATE64=1 -DHAVE_CLOCK_MONOTONIC=1 -DHAVE_WCRTOMB=1 -DHAVE_MBRTOWC=1 -DHAVE_RES_NINIT=1 -DHAVE_GNU_GET_LIBC_VERSION=1 -DHAVE_ICONV=1 -DVA_COPY=va_copy -DHAVE_VA_COPY=1 -DHAVE_VA_LIST_AS_ARRAY=1 -DHAVE_CPP_EXPLICIT=1 -DHAVE_CPP_TYPENAME=1 -DHAVE_CPP_MODERN_SPECIALIZE_TEMPLATE_SYNTAX=1 -DHAVE_CPP_PARTIAL_SPECIALIZATION=1 -DHAVE_CPP_ACCESS_CHANGING_USING=1 -DHAVE_CPP_AMBIGUITY_RESOLVING_USING=1 -DHAVE_CPP_NAMESPACE_STD=1 -DHAVE_CPP_UNAMBIGUOUS_STD_NOTEQUAL=1 -DHAVE_CPP_NEW_CASTS=1 -DHAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR=1 -DNEED_CPP_UNUSED_IMPLEMENTATIONS=1 -DHAVE_THREAD_TLS_KEYWORD=1 -DMALLOC_H=\<malloc.h\> -DHAVE_STRNDUP=1 -DHAVE_POSIX_MEMALIGN=1 -DHAVE_MEMALIGN=1 -DHAVE_VALLOC=1 -DHAVE_I18N_LC_MESSAGES=1 -DHAVE_LOCALECONV=1 -DNS_ALWAYS_INLINE=__attribute__\(\(always_inline\)\) -DNS_ATTR_MALLOC=__attribute__\(\(malloc\)\) -DNS_WARN_UNUSED_RESULT=__attribute__\(\(warn_unused_result\)\) -DNS_NORETURN=__attribute__\(\(noreturn\)\) -DMOZ_DEBUG_SYMBOLS=1 -DJSGC_TESTPILOT=1 -DHAVE___CXA_DEMANGLE=1 -DHAVE__UNWIND_BACKTRACE=1 -DHAVE_TM_ZONE_TM_GMTOFF=1 -DCPP_THROW_NEW=throw\(\) -DEDITLINE=1 -DMOZ_DLL_SUFFIX=\".so\" -DHAVE_POSIX_FALLOCATE=1 -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -DHAVE_SETLOCALE=1 -DHAVE_LOCALECONV=1  \
> +	$< | $(PERL) ./config/make-system-wrappers.pl $(@D)

Obviously we wouldn't want to ship this with that hardcoded list ;-)

::: js/src/assembler/Rules.mk
@@ +3,5 @@
> +# Rules for Assembler
> +
> +OBJ_$(d) :=
> +
> +include build/footer.mk

Is this file actually needed or is this just a placeholder?

::: js/src/build/header.mk
@@ +1,4 @@
> +sp             := $(sp).x
> +dirstack_$(sp) := $(d)
> +d              := $(dir)
> +b              := $(BUILD_DIR)/$(d)

Comments would be great here.  It looks like d is the directory in the objdir, and b is the directory in the srcdir?

::: js/src/editline/Rules.mk
@@ +10,5 @@
> +
> +$(OBJ_$(d)): COMPILE_CFLAGS += -DANSI_ARROWS -DHAVE_TCGETATTR -DHIDE \
> +	-DUSE_DIRENT -DSYS_UNIX -DHAVE_STDLIB -DUNIQUE_HISTORY
> +
> +$(b)/%.o: $(d)/%.c $(SYSTEM_WRAPPERS_JS)/unix.h

This is the kind of thing we'll want to split out into its own variable (e.g. EXTRA_DEPS_$(d) = $(SYSTEM_WRAPPERS_JS)/unix.h or something similar).

::: js/src/fast.mk
@@ +75,5 @@
> +include $(dir)/Rules.mk
> +
> +# TODO: implement Nitro assembler detection
> +dir := assembler
> +include $(dir)/Rules.mk

This is the kind of thing that screams "write a macro".

::: js/src/methodjit/Rules.mk
@@ +4,5 @@
> +
> +# TODO: replace this expression with all filename.
> +OBJ_$(d) := $(patsubst $(d)/%.cpp,$(b)/%.o,$(wildcard $(d)/*.cpp))
> +
> +OBJ_LIBMOZJS += $(OBJ_$(d))

I'm a little confused by all this.  We're doing object files for .cpps but source files for assembly?
Attachment #558827 - Flags: feedback?(khuey)
(Assignee)

Comment 10

7 months ago
This is a really old bug that is no longer relevant to the current JS engine build system.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.