Rdar 30961871 metrics mark 2 #8477

graydon · 2017-04-01T06:18:34Z

This is a second attempt at #8178, taking a different tack by avoiding parseable-output altogether (the integration on the Xcode side was going to be more of a chore, and less diagnostically useful, than I'd hoped; and telemetry on it isn't going to work out this time around).

The goal here is to make a simple, fixed command-line flag that a user can put in a build setting like OTHER_SWIFT_FLAGS="-stats-output-dir /tmp/stats" and have all the driver and frontend invocations that follow on in their build write out all the useful stats and timers their compiler has available (consolidating several pre-existing options), putting the results in a bunch of unique and informatively-named JSON files in the provided dir. These can then be read-back by diagnostic or regression-testing tools, or bundled up and attached to performance bugs.

The patch is also intended to be minimally invasive and relatively easy to port to other branches (including older compilers, for comparison testing). It does not remove any of the existing infrastructure, and in fact leans as much as possible on LLVM's existing statistic and timer classes.

It does depend on a (now-merged) LLVM change (https://reviews.llvm.org/D31566). I tried to make it have zero changes, but LLVM only makes the non-JSON manual entrypoint public, keeping the JSON one private.

rdar://30961871

Summary: This changes the static method TimerGroup::printAllJSONValues from private to public, to match the static method TimerGroup::printAll. When trying to drive the reporting machinery by hand, the existing API is _almost_ flexible enough, but this entrypoint is required to intermix printing timers with other non-timer output. The underlying motive here is a Swift change to consolidate the collection of timers, LLVM statistics and other (non-assert-dependent) counters into JSON files, which requires a bit of manual intervention in LLVM's stat and timer output routines. See swiftlang/swift#8477 for details. Reviewers: MatzeB Reviewed By: MatzeB Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D31566 llvm-svn=299371

Summary: This changes the static method TimerGroup::printAllJSONValues from private to public, to match the static method TimerGroup::printAll. When trying to drive the reporting machinery by hand, the existing API is _almost_ flexible enough, but this entrypoint is required to intermix printing timers with other non-timer output. The underlying motive here is a Swift change to consolidate the collection of timers, LLVM statistics and other (non-assert-dependent) counters into JSON files, which requires a bit of manual intervention in LLVM's stat and timer output routines. See swiftlang/swift#8477 for details. Reviewers: MatzeB Reviewed By: MatzeB Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D31566 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@299371 91177308-0d34-0410-b5e6-96231b3b80d8

graydon · 2017-04-03T21:32:24Z

@swift-ci please smoke test

jrose-apple

Hm. Some comments.

jrose-apple · 2017-04-03T22:14:10Z

lib/Serialization/Serialization.cpp

@@ -4525,7 +4525,7 @@ void swift::serialize(ModuleOrSourceFile DC,

  bool hadError = withOutputFile(getContext(DC), options.OutputPath,
                                 [&](raw_ostream &out) {
-    SharedTimer timer("Serialization (swiftmodule)");
+    SharedTimer timer("Serialization, swiftmodule");


This really shouldn't be necessary. There's already an llvm::yaml::escape.

There is, but LLVM has an assertion that the string is non-escape-requiring. Currently if you run plain-old swift with timers, asserts on, and LLVM's JSON output, it fails.

Bizarre. Okay, I'll assume that was intentional. Is that at least in a doc comment somewhere? :-/

Not sure what the assertion is really trying to get at. It says what it's checking but provides no rationale (aside from the string in question being in quotes already, in JSON; maybe the author was trying to check that the string wasn't going to prematurely end the quote?)

Support/Timer.cpp

assert(!yaml::needsQuotes(Name) && "TimerGroup name needs no quotes"); assert(!yaml::needsQuotes(R.Name) && "Timer name needs no quotes");

Anyway, as with the rest of this patch, I'm on a deadline to get all this stuff working and wanted to keep it as non-invasive as possible. I talked to Matthias about the bit I did need to change in LLVM and his response was along the lines of "I'm going to rewrite all the stats-emitting stuff soon anyways", so I wouldn't spend time polishing it at this point.

jrose-apple · 2017-04-03T22:15:40Z

lib/Basic/Statistic.cpp

+  stream << "stats"
+         << "-" << now.time_since_epoch().count()
+         << "-" << ProcessName
+         << "-" << Process::GetRandomNumber()


PID? It's already an invocation-specific folder, right?

::getpid is unistd.h, not on windows. There's a a _getpid in process.h, but then I'd be adding these things to LLVM's Process.inc files and I wanted to keep the patch as small as possible.

Whoops, hm, okay. I guess you have the primary file name in there (elsewhere).

jrose-apple · 2017-04-03T22:17:30Z

include/swift/Basic/Statistic.h

+  void performedSemanticAnalysis(CompilerInstance &instance) override;
+  void performedSILGeneration(SILModule &module) override;
+  void performedSILDiagnostics(SILModule &module) override;
+  void performedSILOptimization(SILModule &module) override;


Nitpick: const (possibly on the CompilerInstance too)

These are inherited methods from the FrontendObserver, I'm not able to change their type signature (am I?).

Ah, hm, I don't know. I missed that somehow, though, sorry.

jrose-apple · 2017-04-03T22:19:31Z

include/swift/Driver/DependencyGraph.h


    friend class DependencyGraphImpl;
  protected:
-    MarkTracerImpl();
+    MarkTracerImpl(UnifiedStatsReporter *Stats);


Nitpick: please use explicit for single-argument constructors.

Sure, fixed.

…not fixed?

Haha I did fix it in one place (the subclass). Just not two places.

Brains. Sorry! Pushed ever-more-fixy fix.

jrose-apple · 2017-04-03T22:21:57Z

include/swift/Basic/Statistic.h

 #include "llvm/ADT/Statistic.h"
+#include "swift/Basic/Timer.h"
+#include "swift/FrontendTool/FrontendTool.h"


This is a layering violation. Headers in Basic can't import things from FrontendTool. (Basic is actually the bottom layer, so they can't import anything except llvm/ADT and llvm/Support.)

Tricky. How best to make this interface accept callbacks of the appropriate type, then? Or should I move this class out of Basic and into something higher level (eg. FrontendTool itself? Is it appropriate for Driver to see that?)

Hm. Driver and FrontendTool both import Frontend, but neither imports the other, and they certainly can't both import each other. In your case you might be able to get away with forward declarations for the reference types, but the dependency kind won't work.

Really I'm unhappy with the recording code living inside the stats mechanism at all. The end result is that the implementation file ends up with tons of references to the internal structure of several different subsystems. It's weird to see the Driver's DependencyKind mixed in with counting various SIL entities. I'd prefer if it were inverted and the various members incremented directly.

I'm including FrontendTool.h for the definition of FrontendObserver (so that I can implement it), not the forward declarations of its parameters.

If you'd prefer, I could make the Basic/Statistic.h interface expose the full definitions of the AlwaysOnFrontendCounters and AlwaysOnDriverCounters structs, and then implement the incrementing logic in each of Driver and Frontend (by implementing a special-purpose FrontendObserver subclass in FrontendTool itself, say). That would fix the layering violation, at least in the formal sense.

(Longer term I totally agree with you that I don't like all this information leakage between modules; but the LLVM global-static counters remain too expensive to be globally turned-on, and Matthias hasn't convinced LLVM folks of a suitable replacement strategy yet. We could implement our own global-static registry system for counters, it just seemed to me like that would be a bit overkill.)

I like that better. I think it's the best way to satisfy the letter of the layering laws for now. I wouldn't want to make more global stats anyway because of SourceKit eventually having threads.

Eh, you know, if I am going that way .. I didn't wind up relying on more than 2 methods in FrontendObserver and they're both called within FrontendTool.cpp I can avoid routing through it and just call two local, explicit count-the-things functions in FrontendTool.cpp.

graydon · 2017-04-04T00:23:05Z

Updated to be simpler-and-smaller, let the modules do their own counting.

graydon · 2017-04-04T00:25:54Z

@swift-ci please smoke test

graydon · 2017-04-04T00:37:46Z

@swift-ci please smoke test
(minor gcc fix)

jrose-apple

No further comments! (except the one nitpick)

jrose-apple · 2017-04-04T00:53:51Z

lib/Driver/Compilation.cpp

@@ -229,7 +232,13 @@ namespace driver {
                     << ": " << LogJob(Cmd) << "\n";
      }
      FinishedCommands.insert(Cmd);
-
+      if (Comp.Stats) {
+          auto &D = Comp.Stats->getDriverCounters();


Nitpick: funky indentation here.

Whoops. Fixed.

graydon · 2017-04-04T01:09:50Z

@swift-ci please smoke test and merge

graydon · 2017-04-04T01:19:09Z

@swift-ci please smoke test and merge

graydon · 2017-04-04T03:20:06Z

@swift-ci please smoke test and merge

Summary: This changes the static method TimerGroup::printAllJSONValues from private to public, to match the static method TimerGroup::printAll. When trying to drive the reporting machinery by hand, the existing API is _almost_ flexible enough, but this entrypoint is required to intermix printing timers with other non-timer output. The underlying motive here is a Swift change to consolidate the collection of timers, LLVM statistics and other (non-assert-dependent) counters into JSON files, which requires a bit of manual intervention in LLVM's stat and timer output routines. See swiftlang/swift#8477 for details. Reviewers: MatzeB Reviewed By: MatzeB Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D31566 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@299371 91177308-0d34-0410-b5e6-96231b3b80d8

[Timers] Avoid YAML-special characters in timer names.

7c4eea6

graydon requested review from modocache, DougGregor and jrose-apple April 1, 2017 06:19

graydon mentioned this pull request Apr 1, 2017

DO NOT MERGE YET: Rdar 30961871 metrics #8178

Closed

jrose-apple reviewed Apr 3, 2017

View reviewed changes

[Stats] Add UnifiedStatsReporter class.

a30f173

graydon force-pushed the rdar-30961871-metrics-mark-2 branch from 44a816e to ecfcfe0 Compare April 4, 2017 00:21

graydon force-pushed the rdar-30961871-metrics-mark-2 branch from ecfcfe0 to f82a842 Compare April 4, 2017 00:37

graydon force-pushed the rdar-30961871-metrics-mark-2 branch from f82a842 to c72621c Compare April 4, 2017 01:01

jrose-apple approved these changes Apr 4, 2017

View reviewed changes

[Stats] Add -stats-output-dir, wire UnifiedStatsReporter into place.

0245c83

graydon force-pushed the rdar-30961871-metrics-mark-2 branch from c72621c to 0245c83 Compare April 4, 2017 01:09

swift-ci merged commit c39ffda into swiftlang:master Apr 4, 2017

graydon mentioned this pull request Apr 28, 2017

Add an assortment of new "always-on" metrics. #9110

Merged

graydon deleted the rdar-30961871-metrics-mark-2 branch September 13, 2017 16:42

Rdar 30961871 metrics mark 2 #8477

Rdar 30961871 metrics mark 2 #8477

Uh oh!

Conversation

graydon commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graydon commented Apr 3, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graydon commented Apr 4, 2017

Uh oh!

graydon commented Apr 4, 2017

Uh oh!

graydon commented Apr 4, 2017

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!

graydon commented Apr 4, 2017

Uh oh!

graydon commented Apr 4, 2017

Uh oh!

graydon commented Apr 4, 2017

Uh oh!

Uh oh!

graydon commented Apr 1, 2017 •

edited

Loading