Extend embuilder deferred building mode to ports #23924

dschuff · 2025-03-14T00:00:09Z

This allows embuilder build ALL to mix the ports builds together with the
system_libs builds, for even faster build-linux speed.
Headers are still installed eagerly during ports.get() (to allow dependencies to
build their object files) but does not run ninja to build the ports until
the explicit step at the end.

This version requires threading the 'deferred' flag through each port definition.

dschuff · 2025-03-14T00:02:03Z

tools/ports/__init__.py

@@ -36,6 +36,7 @@

 logger = logging.getLogger('ports')

+build_deferred = False


This global variable is ugly but it allows this approach to work without having to edit all the ports definition files. The alternative would be to thread the deferred argument through the get() of each port and its calls of ports.build_port.

dschuff · 2025-03-14T00:02:59Z

tools/cache.py

@@ -177,8 +177,6 @@ def get(shortname, creator, what=None, force=False, quiet=False, deferred=False)
    logger.info(message)
    utils.safe_ensure_dirs(cachename.parent)
    creator(str(cachename))
-    if not deferred:


The removal of this assertion is also a consequence of localizing the change to init.py.

This code is generic to all caching so it seems unfortunate not to be able to assert that the result of a build actually creates the file. Can we revert this part?

We can; I thought it was slightly ugly to thread deferred as an argument through all the port generators so I removed it, but since we put in the env var, we can just use that instead.

But this still isn't great since we are saying that we don't care about the result of any cache access when EMBUILDER_PORT_BUILD_DEFERRED is set.

I guess its better than not checking at all though.

Are you sure we need this, given that port.get is not longer called from add_cflags at all?

Yeah, this also affects non-port system libs builds (it's why this assertion was already conditional and not enabled with deferred builds before this PR). If we wanted something equivalent for deferred builds, we'd have to put code somewhere (here, presumably) to accumulate the list of deferred files we eventually want to see in the cache and verify that they all exist after the ninja invocation finishes. We could maybe introduce an API like cache.verify_deferred or something like that.

tools/ports/__init__.py

…ries

Split out from emscripten-core#23924

dschuff · 2025-03-14T23:11:54Z

tools/ports/sdl2_gfx.py

@@ -16,8 +16,6 @@ def needed(settings):


 def get(ports, settings, shared):
-  sdl_build = os.path.join(ports.get_build_dir(), 'sdl2')
-  assert os.path.exists(sdl_build), 'You must use SDL2 to use SDL2_gfx'


This doesn't work when the actual library build is deferred, but the dependency code in ports/init.py should ensure that the dependency is met.

dschuff · 2025-03-15T00:15:35Z

This does seem to work now, and cut about 4 minutes off of the build-linux stage on the critical path. seems like it might be worthwhile to consider?

Split out from emscripten-core#23924

Split out from #23924

sbc100 · 2025-03-14T18:36:39Z

.circleci/config.yml

@@ -499,7 +499,7 @@ jobs:
      - build-libs
      - run:
          name: Clean build directory
-          command: rm -rf ~/cache/build
+          command: rm -rf ~/cache/build ~/cache/ports-builds


Should we make this directory cache/build/ports instead?

I could go either way on that. After #23932 they are all just together under /build in their own uniquely-named directory, which seems fine to me.

embuilder.py

sbc100 · 2025-03-14T18:44:25Z

tools/cache.py

@@ -129,6 +129,10 @@ def get_lib_dir(absolute):
  return path


+def get_lib_subdir():
+  return get_lib_dir(False).relative_to(Path(get_sysroot(False), 'lib'))


Could we do this the other way around? i.e put the subdir calculation logic here in this function and then have get_lib_dir call get_lib_subdir?

def get_lib_dir(absolute): ensure_setup() libroot = Path(get_sysroot(absolute=absolute), 'lib') return Path(libroot, get_lib_subdir()

This is removed in the current version.

embuilder.py

This allows building them simultaneously with Ninja. Split out from emscripten-core#23924

This allows building them simultaneously with Ninja. Split out from #23924

sbc100 · 2025-03-21T16:57:16Z

tools/ports/__init__.py

@@ -578,9 +584,13 @@ def add_cflags(args, settings): # noqa: U100

  # Now get (i.e. build) the ports in dependency order.  This is important because the
  # headers from one ports might be needed before we can build the next.
+  global build_deferred
+  assert not build_deferred, "add_cflags shouldn't be called from embuilder"
+  build_deferred = True


When are the deferred ports actually built?

If we defer the building of the port does when only compiling does that mean that the port headers will be installed N times if we do emcc -c -sUSE_SDL2 for each source file, and then once again at link time?

Deferred libs and ports are only built when using embuilder.py. It explicitly invokes ninja (via system_libs.build_deferred()) after all the calls to port.build.
emcc never builds anything deferred.

So this code does nothing outside of embuilder?

Is it still needed? I find the assertion a little strange. If add_cflags is never called from embuilder then why are we needing to set build_deferred here (which means nothing outside of embuilder).

Ah yes, so here's the issue. The headers for any dependencies need to be in place in order to build object files. So (as the comment on top of the function suggests), this can normally cause those dependencies to be built to get the headers in place. When building with embuilder we want all the object files to be deferred and built by the single invocation of ninja at the end (i.e. we don't want the invocations of emcc that build the object files to cause additional object files and archives to be built). But emcc is in a different process from embuilder, so it doesn't have access to the build_deferred global in the embuilder process. So what was happening was that some of the libraries were getting built twice (once as a result of add_cflags inside an invocation of emcc, and once from the top-level ninja file).

So this use of build_deferred is for emcc usage and not for embuilder usage?

So emcc always installs headers in build_deffered mode now?

Doesn't that mean that if I compile 1000 source files I will get the headers for any ports use installed 1000 separate times? Since none of the compile steps actually build the library (due to being deffered) each compile will run as if the library is completely missing and go ahead and install the headers, no?

Locally it doesn't seem like cache lock contention is too much of an issue (after the port is unpacked, which only happens ones) but I guess the create function is called under the lock, so the header check happens with the lock. Another env variable would certainly do the trick. It could be scoped to skipping ports as you suggested, or it could be used more generally to mean that we are running an embuilder library build. It coudl just replace the build_deferred global variable.

Its too gross for me I think.

At the very least we should somehow ensure this behaviour change is only for embuilder. As it stands this would effect all USE_NINJA=1 users. How about a separate USE_DEFFERED_BUILD=1 or something like that that embuilder can set?

Yeah, I was initially not thrilled about another env var, but the fact that it can replace all the uses of this global is nice. I'll go with that.

I thought about this a little more. Using the env var in place of the global var here does make it a bit nicer. But I realized that we don't actually need that in order to keep the headers from being installed redundantly. we just need to omit the ports.get for the dependency ports under embuilder. It's nicer.

I think this works; WDYT?

sbc100 · 2025-03-31T20:51:05Z

embuilder.py

@@ -280,6 +281,9 @@ def main():
  if auto_tasks:
    print('Building targets: %s' % ' '.join(tasks))

+  if USE_NINJA:
+    os.environ['EMBUILDER_PORT_BUILD_DEFERRED'] = '1'


So its not currently possible use ninja in non-deferred mode? I guess thats OK?

Ninja still works in non-deferred mode. If you do EMCC_USE_NINJA=1 test/runner.py core0 it will build the libraries on-demand in non-deferred mode with ninja. Deferred mode only makes sense when using embuilder though.

sbc100 · 2025-03-31T20:52:46Z

tools/ports/__init__.py

-    port.get(Ports, settings, shared)
+    # When using embuilder, don't build the dependencies
+    if not os.getenv('EMBUILDER_PORT_BUILD_DEFERRED'):
+      port.get(Ports, settings, shared)


I'm not sure this will work either since what if I do emcc -sUSE_SDL=2 -c foo.c.. in this case I would want at least the headers to be installed at this point/

Oh I see, in that case EMBUILDER_PORT_BUILD_DEFERRED would never be set...

Right. when not using embuilder, everything will build non-deferred on demand, one at a time, just like what currently happens.

dschuff · 2025-04-04T00:31:48Z

Any more thoughts or questions on this one?

dschuff added 2 commits March 13, 2025 18:34

Allow deferred/combined building of ports with system_libs.

021e0cc

This version requires threading the 'deferred' flag through each port definition.

use a global in cache/__init__.py instead

ba2b0d6

dschuff commented Mar 14, 2025

View reviewed changes

Use separate build directories for ports variants

f6e3bdd

dschuff commented Mar 14, 2025

View reviewed changes

tools/ports/__init__.py Outdated Show resolved Hide resolved

dschuff added 2 commits March 14, 2025 18:39

clean up, work around lack of str.removeprefix

eb3101c

Use lib archive path/name instead of variant name to separate directo…

830eddb

…ries

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 14, 2025

Use common build directory for ports and system libs. NFC

eb5c5bc

Split out from emscripten-core#23924

sbc100 mentioned this pull request Mar 14, 2025

Use common build directory for ports and system libs. NFC #23932

Merged

dschuff commented Mar 14, 2025

View reviewed changes

remove unused argument

7082238

dschuff marked this pull request as ready for review March 15, 2025 00:15

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 17, 2025

Use common build directory for ports and system libs. NFC

e0b8410

Split out from emscripten-core#23924

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 17, 2025

Use common build directory for ports and system libs. NFC

f95ec90

Split out from emscripten-core#23924

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 19, 2025

Use common build directory for ports and system libs. NFC

49bce1a

Split out from emscripten-core#23924

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 19, 2025

Use common build directory for ports and system libs. NFC

33114f1

Split out from emscripten-core#23924

sbc100 added a commit that referenced this pull request Mar 19, 2025

Use common build directory for ports and system libs. NFC (#23932)

5317e44

Split out from #23924

dschuff added 3 commits March 20, 2025 18:04

Merge branch 'main' into ports-deferred

060947e

remove unnecessary file deletion

0ad6f2c

fix ruff

36090e3

sbc100 reviewed Mar 20, 2025

View reviewed changes

remove unneeded ensure_dirs

94648d8

dschuff added a commit to dschuff/emscripten that referenced this pull request Mar 20, 2025

Use a separate build directory for each port

03c1930

This allows building them simultaneously with Ninja. Split out from emscripten-core#23924

dschuff mentioned this pull request Mar 20, 2025

Use a separate build directory for each port #23961

Merged

dschuff added a commit that referenced this pull request Mar 21, 2025

Use a separate build directory for each port (#23961)

a5af114

This allows building them simultaneously with Ninja. Split out from #23924

Merge branch 'main' into ports-deferred

6d36a65

sbc100 reviewed Mar 21, 2025

View reviewed changes

Use global

75b1151

dschuff added 4 commits March 25, 2025 23:44

skip ports.get to avoid redundant header install

3e52a80

skip ports.get when embuilder

ca3585e

Merge branch 'main' into ports-deferred

a4d161e

Merge branch 'main' into ports-deferred

031ff61

sbc100 reviewed Mar 31, 2025

View reviewed changes

Merge branch 'main' into ports-deferred

6faa8a5

dschuff force-pushed the ports-deferred branch from e68e0db to 6faa8a5 Compare April 3, 2025 01:02

Put back assertion

1772acd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend embuilder deferred building mode to ports #23924

Extend embuilder deferred building mode to ports #23924

dschuff commented Mar 14, 2025 •

edited

Loading

dschuff Mar 14, 2025

dschuff Mar 14, 2025

sbc100 Apr 7, 2025

dschuff Apr 7, 2025

sbc100 Apr 7, 2025

dschuff Apr 7, 2025

dschuff Mar 14, 2025

dschuff commented Mar 15, 2025

sbc100 Mar 14, 2025

dschuff Mar 20, 2025

sbc100 Mar 14, 2025

dschuff Mar 20, 2025

sbc100 Mar 21, 2025

dschuff Mar 21, 2025

sbc100 Mar 21, 2025

dschuff Mar 21, 2025

sbc100 Mar 21, 2025

dschuff Mar 22, 2025

sbc100 Mar 22, 2025

dschuff Mar 22, 2025 •

edited

Loading

dschuff Mar 25, 2025

dschuff Mar 26, 2025

sbc100 Mar 31, 2025

dschuff Apr 2, 2025

sbc100 Mar 31, 2025

sbc100 Mar 31, 2025

dschuff Apr 2, 2025

dschuff commented Apr 4, 2025

		@@ -36,6 +36,7 @@

		logger = logging.getLogger('ports')

		build_deferred = False

Extend embuilder deferred building mode to ports #23924

Are you sure you want to change the base?

Extend embuilder deferred building mode to ports #23924

Conversation

dschuff commented Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Mar 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Apr 4, 2025

dschuff commented Mar 14, 2025 •

edited

Loading

dschuff Mar 22, 2025 •

edited

Loading