Change stdlib swiftc's to depend on .swiftmodule's instead of .o's #5472

cooperp · 2016-10-26T02:29:08Z

Currently SwiftOnoneSupport.o waits on Swift.o to finish compiling before we start
compiling SwiftOnoneSupport.o. We don't need the .o for compilation but instead just
need Swift.swiftmodule which we were generating anyway.

This change causes .o and { .swiftmodule, .swiftdoc } to be built independently of each
other. Downstream compiles only wait on the module while downstream links wait on the .o.

Given that .apinotes are sometimes used by swiftc invocations, this also separates out
apinotes in to its own step which these other commands can both depend on.

The result of this is a 30% improvement in building the stdlib with -R and 20% with -d.
Specifically, -R drops from 3m to 2m and -d drops from 12m to just under 10m.

This was tested with --validation-test which uncovered a missing dependency. After adding that
in, we now pass the tests.

DougGregor · 2016-10-26T03:22:42Z

Fantastic!

DougGregor · 2016-10-26T03:23:12Z

@swift-ci please smoke test

rudkx · 2016-10-26T03:23:19Z

Yay! Thanks Pete.

DougGregor · 2016-10-26T05:20:50Z

Would love a review from someone more familiar with our CMake setup.

jrose-apple

It pretty much all looks fine to me; just a few comments here and there. I hadn't even thought to come at this from this side because I was still thinking of type-checking as the main slowdown. This does do some extra work and spawns more processes, but with an optimized stdlib it's probably a win. (Less sure about a debug compiler / debug stdlib combination, but that's not very common.)

Thanks, Pete!

jrose-apple · 2016-10-26T15:45:27Z

cmake/modules/AddSwift.cmake

@@ -1320,6 +1329,8 @@ function(add_swift_library name)
        # Configure variables for this subdirectory.
        set(VARIANT_SUFFIX "-${SWIFT_SDK_${sdk}_LIB_SUBDIR}-${arch}")
        set(VARIANT_NAME "${name}${VARIANT_SUFFIX}")
+        set(MODULE_VARIANT_SUFFIX "-swiftmodule-${SWIFT_SDK_${sdk}_LIB_SUBDIR}-${arch}")


Should it just get this from the regular VARIANT_SUFFIX?

Do you mean the "${SWIFT_SDK_${sdk}_LIB_SUBDIR}-${arch}" part in MODULE_VARIANT_SUFFIX? If so, then yeah, I'll fix that.

jrose-apple · 2016-10-26T15:46:51Z

cmake/modules/AddSwift.cmake

@@ -1384,6 +1389,15 @@ function(add_swift_library name)
          endif()
        endforeach()

+        set(swiftlib_framework_depends_flattened ${SWIFTLIB_FRAMEWORK_DEPENDS})


Why move this?

And if it is just a move, can you do the move in a separate commit?

Actually it didn't move. It the diff not really knowing how I applied the changes.

What we have before was a loop to set swiftlib_module_dependency_targets, then later a loop to set swiftlib_framework_depends_flattened, and finally a loop for swiftlib_private_link_libraries_targets.

In the original code, swiftlib_private_link_libraries_targets was initialized to swiftlib_module_dependency_targets, but we don't want that any more. This is because swiftlib_module_dependency_targets now contains references to the swift module files, and we want swiftlib_private_link_libraries_targets to reference the linked images. So I changed swiftlib_private_link_libraries_targets to initialise to empty instead of swiftlib_module_dependency_targets then added appends to this loop to set it appropriately.

And yes, I could definitely move the in a separate commit then only make the change the following a change in a new commit:

list(APPEND swiftlib_module_dependency_targets
"swift${mod}${VARIANT_SUFFIX}")

to

list(APPEND swiftlib_module_dependency_targets
"swift${mod}${MODULE_VARIANT_SUFFIX}")

There definitely is a move here: the ICU section and the swiftlib_framework_depends_flattened switched. I do see that it makes a little more sense.

Sorry, yes there's a move. I still had the 'git diff' my local machine shows in my head. In it, it appears that the loop over swiftlib_framework_depends_flattened moves, which it didn't really. What actually moved was the content I described above.

jrose-apple · 2016-10-26T15:47:54Z

cmake/modules/SwiftSource.cmake

+      list(APPEND module_command
+          "-emit-module"
+          "-emit-module-path" "${module_file}"
+          "-o" "${module_file}")


You shouldn't need both -emit-module-path and -o here. Either one should be fine.

Oddly it didn't work with only -emit-module and no -o. No error or anything, just a lack of anything being written to disk. If I can remove the -emit-module-path then that would be fine for now. I should file a bug for the lack of -o causing there to be no writes to disk and no errors.

jrose-apple · 2016-10-26T15:51:34Z

cmake/modules/SwiftSource.cmake

+  if (NOT ${num_apinotes_outputs} EQUAL 0)
+    add_custom_command_target(
+        api_notes_dependency_target
+        ${command_create_dirs}


Creating directories needs to be done regardless, no?

Yep, thats a mistake. Good catch. I might just always have this target run, but in some cases it will only create directories and in others it will create directories and api notes. If you have a preference for something else i'm also totally fine with alternatives.

That seems good enough. Creating API notes is very cheap.

jrose-apple · 2016-10-26T15:51:36Z

cmake/modules/SwiftSource.cmake

+  endif()
+
+  # First generate the api notes if we need them.
+  list(LENGTH command_create_apinotes num_apinotes_outputs)


Should this be measuring apinotes_outputs instead?

Is there any reason to not just use if (apinotes_outputs)?

if (apinotes_outputs) is much better. I'll use that. I just don't know enough CMake to know if thats ok or not. I figured it would be on a string but wasn't sure about a list.

jrose-apple · 2016-10-26T15:52:13Z

cmake/modules/SwiftSource.cmake

+        DEPENDS
+          ${swift_compiler_tool_dep}
+          ${source_files} ${SWIFTFILE_DEPENDS}
+          ${swift_ide_test_dependency} ${depends_create_apinotes}


This should actually only depend on depends_create_apinotes. API notes are completely independent from any of the Swift stuff.

Good catch. Will fix that.

jrose-apple · 2016-10-26T15:53:19Z

cmake/modules/SwiftSource.cmake

+        ${swift_compiler_tool_dep}
+        ${source_files} ${SWIFTFILE_DEPENDS}
+        ${swift_ide_test_dependency} ${api_notes_dependency_target}
+      COMMENT "Compiling ${first_output}")


Different comment, maybe?

Yep, I should put in "Generating {module_name}" instead.

gottesmm

Meta request. Can you make sure that before/after if you do a clean build nothing is rebuilt. From what I understand ChrisB got us to that point and I would like to stay that way = ).

gottesmm · 2016-10-26T16:30:42Z

cmake/modules/AddSwift.cmake

@@ -536,7 +536,7 @@ endfunction()
 #
 # source1 ...
 #   Sources to add into this library
-function(_add_swift_library_single target name)
+function(_add_swift_library_single target module_target name)


Can you add documentation to the header for what module_target is. Also, even better, can you change these options here to be cmake_parse_argument Arguments. Then below when you use "" for the module_target name, it would be at least self documenting that it is not a bug (see question below).

Sure, will add documentation.

I can try out adding a parse argument. Will need to also change 'target' above as I only added 'module_target' in this way to be consistent.

Would you prefer that change first, then land the rest of this?

gottesmm · 2016-10-26T16:32:58Z

cmake/modules/AddSwift.cmake

@@ -1384,6 +1389,15 @@ function(add_swift_library name)
          endif()
        endforeach()

+        set(swiftlib_framework_depends_flattened ${SWIFTLIB_FRAMEWORK_DEPENDS})


And if it is just a move, can you do the move in a separate commit?

gottesmm · 2016-10-26T16:38:20Z

cmake/modules/AddSwift.cmake

@@ -700,6 +701,13 @@ function(_add_swift_library_single target name)
      INSTALL_IN_COMPONENT "${SWIFTLIB_INSTALL_IN_COMPONENT}")
  add_swift_source_group("${SWIFTLIB_SINGLE_EXTERNAL_SOURCES}")

+  # If there were any swift sources, then a .swiftmodule may have been created.
+  # If that is the case, then add a target which is an alias of the module files.
+  if(NOT "${module_target}" STREQUAL "" AND NOT "${swift_module_dependency_target}" STREQUAL "")


If you made module_target a cmake_parse_arguments target, then this would be cleaner. Specifically, you could do:

if (NOT "${SWIFTLIB_MODULE_TARGET}" AND NOT "${swift_module_dependency_target}")

endif()

Then it would be clear that module_target is an argument due to the SWIFTLIB_* prefix.

I see. Yeah the prefix would be better than what we have today.

gottesmm · 2016-10-26T16:39:23Z

cmake/modules/AddSwift.cmake

        set(swiftlib_module_dependency_targets)
+        set(swiftlib_private_link_libraries_targets)


Unless I am missing something, isn't the variable swiftlib_private_link_libraries_targets dead?

Didn't see to be. I had issues with this when I was writing the patch. Ultimately it makes its way to the following which causes dylibs to be on the link line:

function(_add_swift_library_single
...
target_link_libraries("${target}" PRIVATE
${SWIFTLIB_SINGLE_PRIVATE_LINK_LIBRARIES})

Right, it's still a dependency of the link target for each module.

gottesmm · 2016-10-26T16:40:44Z

cmake/modules/AddSwift.cmake

@@ -1603,6 +1618,7 @@ function(add_swift_library name)

    _add_swift_library_single(
      ${name}
+      ""


This is why I would suggest adding a named argument /assuming/ that is the correct thing to do here.

Then it would be clear that this is not a bug since you would have:

MODULE_TARGET ""

That being said, if you do what I suggested above, then this argument could be removed.

Ah yeah, i like the self documentation there. Much better.

gottesmm · 2016-10-26T16:42:48Z

cmake/modules/SwiftSource.cmake

@@ -274,6 +280,7 @@ function(_compile_swift_files dependency_target_out_var_name)
  set(module_file)
  set(module_doc_file)

+  set(module_command)
  if(NOT SWIFTFILE_IS_MAIN)


This section may make sense to refactor into its own function... thoughts?

My reasoning is that in the general flow of things, adding swift_flags -parse-as-library where you adding it, seems funky. That being said, this isn't that big of a deal.

It was actually just a move as the previous code had use adding '-emit-module ... -parse-as-library' on one branch and '-parse-as-library' on the other. But I agree that this is getting a little unwieldy at its current size. Thats probably worth someone refactoring a whole bunch of stuff though, which is better left to who is going to own it long term.

Ok. I am fine with that.

gottesmm · 2016-10-31T21:07:30Z

cmake/modules/AddSwift.cmake

      ${name}
      ${SWIFTLIB_SHARED_keyword}
      ${SWIFTLIB_STATIC_keyword}
      ${SWIFTLIB_OBJECT_LIBRARY_keyword}
      ${SWIFTLIB_SOURCES}
+      MODULE_TARGET ""


This should be no longer necessary.

Thanks. You're right. Just removed it.

gottesmm · 2016-10-31T21:20:44Z

I think you have answered all of my concerns. Removing my request.

gottesmm · 2016-10-31T21:21:19Z

Before you commit though, can you do a rebase/squash to prettify the history on your branch?

cooperp · 2016-10-31T21:23:52Z

Thanks Michael! Yeah, will squash the commits and make it all pretty.

Should I do one last swift-ci just to verify, or does that get done automatically for me when the PR is merged?

…wiftc deps. Currently SwiftOnoneSupport.o waits on Swift.o to finish compiling before we start compiling SwiftOnoneSupport.o. We don't need the .o for compilation but instead just need Swift.swiftmodule which we were generating anyway. This change causes .o and { .swiftmodule, .swiftdoc } to be built independently of each other. Downstream compiles only wait on the module while downstream links wait on the .o. Given that .apinotes are sometimes used by swiftc invocations, this also separates out apinotes in to its own step which these other commands can both depend on. The result of this is a 30% improvement in building the stdlib with -R and 20% with -d. Specifically, -R drops from 3m to 2m and -d drops from 12m to just under 10m.

cooperp · 2016-11-01T17:49:22Z

@swift-ci Please test and merge

…rating module command. We need to actually have a module name and outputs or else the add_custom_command will try to generate multiple commands all with the name NOTFOUND. Note, this bug was introduced in 5834df3 via PR swiftlang#5472.

jrose-apple reviewed Oct 26, 2016

View reviewed changes

gottesmm requested changes Oct 26, 2016

View reviewed changes

gottesmm reviewed Oct 31, 2016

View reviewed changes

gottesmm approved these changes Oct 31, 2016

View reviewed changes

cooperp force-pushed the cmake-swiftmodule branch from 6a8722b to 53ecbf2 Compare October 31, 2016 21:27

cooperp merged commit 5834df3 into swiftlang:master Nov 1, 2016

cooperp mentioned this pull request Nov 1, 2016

Fix cmake by checking for module_name before generating module command #5586

Merged

jrose-apple mentioned this pull request Dec 6, 2016

[CMake] Don't pass -embed-bitcode to module generation commands. #6109

Merged

		set(swiftlib_module_dependency_targets)
		set(swiftlib_private_link_libraries_targets)

Change stdlib swiftc's to depend on .swiftmodule's instead of .o's #5472

Change stdlib swiftc's to depend on .swiftmodule's instead of .o's #5472

Uh oh!

Conversation

cooperp commented Oct 26, 2016

Uh oh!

DougGregor commented Oct 26, 2016

Uh oh!

DougGregor commented Oct 26, 2016

Uh oh!

rudkx commented Oct 26, 2016

Uh oh!

DougGregor commented Oct 26, 2016

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!