Batch mode formation minor fixes #15091

graydon · 2018-03-09T01:36:32Z

This fixes a couple minor issues that arose while testing batch mode. Neither is critical to correct functioning, they just help testing.

The first eliminates a quadratic assert, which I incorrectly assumed was cheaper than a path I thought was already occurring at worse-than-quadratic complexity (relative to batch size). This is not true and to keep things moving quickly on large batches I believe it's not worth making this assertion; it doesn't check a condition that's very plausible anyways.

The second fixes a potential mismatch between the emitted order of primary-file arguments and additional output arguments passed to the frontend. This doesn't happen in normal runs, but when shuffling the batch constituents with -driver-batch-seed -- as happens when doing randomized testing -- it does. And in general it's a condition we ought to be ensuring regardless (at least until we do away with positionally pairing-up primaries and their additionals on the frontend command line).

graydon · 2018-03-09T01:48:21Z

@swift-ci please test

jrose-apple · 2018-03-09T02:06:49Z

lib/Driver/ToolChain.cpp

+sortBatchInputJobs(ArrayRef<const Job *> jobs,
+                   SmallVector<const Job *, 16> &sortedJobs,
+                   Compilation &C) {
+  llvm::StringMap<const Job *> jobsByInput;


Mapping by string seems expensive. Is it not possible to just use the order of jobs before shuffling?

No, but I can perform a shuffle in a way that maintains the order if you prefer. I would like to leave this in as an assert that the orders match, in that case.

Maybe it doesn't matter cause we do tons of other string lookups. This just wasn't the obvious way to get the original order back for me.

swift-ci · 2018-03-09T03:06:53Z

Build failed
Swift Test Linux Platform
Git Sha - 14f3904743d02cbcb469079e046870cf3a7ba17d

swift-ci · 2018-03-09T03:18:12Z

Build failed
Swift Test OS X Platform
Git Sha - 14f3904743d02cbcb469079e046870cf3a7ba17d

graydon · 2018-03-09T04:57:26Z

@jrose-apple pushed update that does the shuffle on indices not jobs, thus maintaining invariant. Switched the comparison-to-the-input sequence to just be an assert.

graydon · 2018-03-09T04:57:35Z

@swift-ci please test

swift-ci · 2018-03-09T04:57:57Z

Build failed
Swift Test Linux Platform
Git Sha - 14f3904743d02cbcb469079e046870cf3a7ba17d

swift-ci · 2018-03-09T04:58:01Z

Build failed
Swift Test OS X Platform
Git Sha - 14f3904743d02cbcb469079e046870cf3a7ba17d

jrose-apple · 2018-03-09T16:42:48Z

lib/Driver/Compilation.cpp

-      for (const Job *Cmd : Batchable) {
-        maybeAdvanceToNextPartition(i, Partition, Batchable.size());
-        std::vector<const Job*> &P = Partition[i];
+      for (size_t i = 0; i < Batchable.size(); ++i) {


If you want, there's an llvm::for_each that can do parallel iteration over two containers.

Assuming you mean swift::for_each (in include/swift/Basic/STLExtras.h), nice!

Oops, forgot it was us and not them.

jrose-apple · 2018-03-09T16:44:57Z

lib/Driver/Compilation.cpp

+        // size of the first Remainder of them.
+        if (P < Remainder)
+          ++TargetSize;
+        for (size_t N = 0; N < TargetSize; ++N) {


You could use std::fill_n here but I guess it may or may not be clearer than just writing it out.

jrose-apple · 2018-03-09T16:46:05Z

test/Driver/batch_mode_aux_file_order.swift

+// RUN: %swiftc_driver -enable-batch-mode -driver-batch-seed 1 -driver-print-jobs -driver-skip-execution -j 3 -emit-module -module-name foo %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/file-04.swift %t/file-05.swift %t/file-06.swift %t/file-07.swift %t/file-08.swift %t/file-09.swift >%t/out.txt
+// RUN: %FileCheck %s <%t/out.txt
+//
+// Each batch should get 3 primaries; check that each has 3 modules _in the same numeric order_.


What are we going to do with this test when we switch to output file maps? Just delete it?

Yes. I can add a note to that extent.

… of inputs.

graydon · 2018-03-09T20:18:20Z

@swift-ci please test and merge

graydon · 2018-03-09T23:07:57Z

@swift-ci please test and merge

graydon · 2018-03-10T01:20:17Z

@swift-ci please test Linux platform

swift-ci · 2018-03-10T01:26:27Z

Build failed
Swift Test Linux Platform
Git Sha - fa830fe

graydon · 2018-03-10T06:58:31Z

@swift-ci please test Linux platform

swift-ci · 2018-03-10T08:13:16Z

Build failed
Swift Test Linux Platform
Git Sha - fa830fe

graydon · 2018-03-12T05:39:23Z

@swift-ci please smoke test and merge

davezarzycki · 2018-03-12T19:51:34Z

The test suite fails after this pull request on Fedora Linux 27 (with ToT clang/llvm). Build log: http://znu.io/pr15091.log

The relevant error:

'BUILD_DIR=/home/dave/s/u/t/tools/swift' --sanitize 'SOURCE_DIR=/home/dave/s/u/tools/swift' --use-filecheck '/home/dave/s/u/t/bin/FileCheck' /home/dave/s/u/tools/swift/test/Driver/batch_mode.swift -check-prefix=SEED1
env SDKROOT= '/home/dave/s/u/t/bin/swiftc' -module-cache-path '/tmp/swift-testsuite-clang-module-cachephl7vb' -swift-version 3  -enable-batch-mode -driver-show-job-lifecycle -driver-skip-execution -j 4 -driver-batch-seed 2 /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-01.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-02.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-03.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-04.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-05.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-06.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-07.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-08.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-09.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-10.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-11.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-12.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-13.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-14.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-15.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-16.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-17.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-18.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-19.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-20.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-21.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-22.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-23.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-24.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-25.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-26.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-27.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-28.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-29.swift /home/dave/s/u/t/tools/swift/test-linux-x86_64/Driver/Output/batch_mode.swift.tmp/file-30.swift | /home/dave/s/u/tools/swift/utils/PathSanitizingFileCheck --sanitize 'BUILD_DIR=/home/dave/s/u/t/tools/swift' --sanitize 'SOURCE_DIR=/home/dave/s/u/tools/swift' --use-filecheck '/home/dave/s/u/t/bin/FileCheck' /home/dave/s/u/tools/swift/test/Driver/batch_mode.swift -check-prefix=SEED2
--
Exit Code: 1

Command Output (stderr):
--
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
Output placeholder
/home/dave/s/u/tools/swift/test/Driver/batch_mode.swift:56:11: error: expected string not found in input
// SEED2: Adding {compile: file-{{.*}} <= file-{{.*}}.swift} to batch 3
          ^
<stdin>:57:1: note: scanning from here
Adding {compile: file-23-9616ac.o <= file-23.swift} to batch 1
^

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 

2 warning(s) in tests.
Testing Time: 166.11s
********************
Failing Tests (1):
    Swift(linux-x86_64) :: Driver/batch_mode.swift

  Expected Passes    : 3029
  Expected Failures  : 80
  Unsupported Tests  : 1384
  Unexpected Failures: 1
FAILED: tools/swift/test/CMakeFiles/check-swift-linux-x86_64 
cd /home/dave/s/u/t/tools/swift/test && /usr/bin/cmake -E remove_directory /home/dave/s/u/t/./swift-test-results/x86_64-unknown-linux-gnu && /usr/bin/cmake -E make_directory /home/dave/s/u/t/./swift-test-results/x86_64-unknown-linux-gnu && /usr/bin/python2.7 /home/dave/s/u/utils/lit/lit.py --incremental -sv --timeout=3000 --xunit-xml-output=/home/dave/s/u/t/./swift-test-results/x86_64-unknown-linux-gnu/lit-tests.xml --param swift_test_subset=primary --param swift_test_mode=optimize_none /home/dave/s/u/t/tools/swift/test-linux-x86_64
ninja: build stopped: subcommand failed.

jrose-apple · 2018-03-12T20:00:15Z

Uh, hm. The random number generation should be deterministic, since the definition minstd_rand is mandated by the standard.

davezarzycki · 2018-03-12T20:10:51Z

Do the Swift Linux build bots use libstdc++ or libcxx?

graydon · 2018-03-12T20:18:43Z

Whatever stock ubuntu is, afaict. But .. well, I'm .. sympathetic to the idea that the prng ought to be stable, since it's spec'ed, but also that I meant the test that fails there to be not-sensitive to PRNG order, and I missed that the failing test here will now be sensitive.

@davezarzycki can you run this test and see if it passes?

https://github.com/google/libcxx/blob/master/test/numerics/rand/rand.predef/minstd_rand.pass.cpp

davezarzycki · 2018-03-12T20:28:53Z

I built it with the same compiler I use to build Swift ToT. It passes.

I also just did a clean build and reinstall of ToT clang+llvm+lld in case it matters, and then did a clean build-and-test of llvm+clang+swift. Same error.

graydon · 2018-03-12T20:39:05Z

@davezarzycki Interesting; I wonder if this has something more to do with a container type behaving differently, like in the job queue or such. Can you grab the actual-output from the failing line of the test (eg. modify test/Driver/batch_mode.swift to redirect the 3rd %swiftc_driver execution -- the SEED2 one -- to a file and pastebin it?)

davezarzycki · 2018-03-12T21:04:04Z

Here you go: http://znu.io/seed2.out

graydon · 2018-03-12T21:41:04Z

@davezarzycki Great, that clarified that it's definitely a different sequence, and that was enough to find the issue: the PRNG is spec'ed tightly enough not to vary this way, but std::shuffle isn't -- it uses the PRNG in a way that leaves some variation in implementation:

libstdc++: https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/stl_algo.h#L3777-L3858
libcxx: https://github.com/llvm-mirror/libcxx/blob/master/include/algorithm#L3197-L3219

Different shuffle algorithms :(

So I need to make the test more order-insensitive. Thanks for catching this!

davezarzycki · 2018-03-12T21:42:31Z

Hi @graydon – Thanks for being so responsive and helpful in debugging this :-)

jrose-apple · 2018-03-12T22:45:23Z

Oh, oops. Once I ruled out the obvious thing I didn't keep thinking. Of course std::shuffle isn't spec'd in the same way.

graydon requested a review from jrose-apple March 9, 2018 01:36

jrose-apple reviewed Mar 9, 2018

View reviewed changes

graydon force-pushed the batch-mode-formation-minor-fixes branch from 14f3904 to 4bdc0cf Compare March 9, 2018 04:49

[BatchMode] Remove a low-value quadratic assertion.

03a9340

graydon force-pushed the batch-mode-formation-minor-fixes branch from 4bdc0cf to 409c23c Compare March 9, 2018 04:56

jrose-apple reviewed Mar 9, 2018

View reviewed changes

[BatchMode] Preserve invariant that shuffled batches are subsequences…

fa830fe

… of inputs.

graydon force-pushed the batch-mode-formation-minor-fixes branch from 409c23c to fa830fe Compare March 9, 2018 18:13

jrose-apple approved these changes Mar 9, 2018

View reviewed changes

swift-ci merged commit 7d16d13 into swiftlang:master Mar 12, 2018

graydon mentioned this pull request Mar 12, 2018

[BatchMode] Fix non-portable test that relies on std::shuffle implementation. #15191

Merged

Batch mode formation minor fixes #15091

Batch mode formation minor fixes #15091

Uh oh!

Conversation

graydon commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graydon commented Mar 9, 2018

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!

swift-ci commented Mar 9, 2018

Uh oh!

swift-ci commented Mar 9, 2018

Uh oh!

graydon commented Mar 9, 2018

Uh oh!

graydon commented Mar 9, 2018

Uh oh!

swift-ci commented Mar 9, 2018

Uh oh!

swift-ci commented Mar 9, 2018

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!

graydon commented Mar 9, 2018

Uh oh!

graydon commented Mar 9, 2018

Uh oh!

graydon commented Mar 10, 2018

Uh oh!

swift-ci commented Mar 10, 2018

Uh oh!

graydon commented Mar 10, 2018

Uh oh!

swift-ci commented Mar 10, 2018

Uh oh!

graydon commented Mar 12, 2018

Uh oh!

davezarzycki commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Mar 12, 2018

Uh oh!

davezarzycki commented Mar 12, 2018

Uh oh!

graydon commented Mar 12, 2018

Uh oh!

davezarzycki commented Mar 12, 2018

Uh oh!

graydon commented Mar 12, 2018

Uh oh!

davezarzycki commented Mar 12, 2018

Uh oh!

graydon commented Mar 12, 2018

Uh oh!

davezarzycki commented Mar 12, 2018

Uh oh!

jrose-apple commented Mar 12, 2018

Uh oh!

Uh oh!

graydon commented Mar 9, 2018 •

edited

Loading

davezarzycki commented Mar 12, 2018 •

edited

Loading