Improve indentation in type checker debugging output #62053

abdulowork · 2022-11-11T11:09:38Z

I was looking at the reason behind the performance bug in #58955 and found that the compiler was spending most of the time in the type checker. While investing, I found the DebugConstraints flag very useful; however, the output of this flag was not always indented, which made the debugging log hard to fold and read.

This PR improves indentation in some of the debugging output.

I noticed the debugging output is mostly untested. Should I try to add tests for the indentation?

abdulowork · 2022-11-12T04:28:49Z

lib/Sema/CSSolver.cpp

@@ -375,8 +377,10 @@ bool ConstraintSystem::simplify() {
      retireFailedConstraint(constraint);
      if (isDebugMode()) {
        auto &log = llvm::errs();
-        log.indent(solverState->getCurrentIndent() + 2) << ")\n";


This would print an unbalanced brace if constrain was ConstraintKind::Disjunction || ConstraintKind::Conjunction

abdulowork · 2022-11-12T04:37:31Z

lib/Sema/CSStep.cpp

-      choice.print(log, &ctx.SourceMgr);
-      log << '\n';
+      choice.print(log, &ctx.SourceMgr, 0, CS.getCurrentIndent());
+      log << ")\n";


This brace was unbalanced

abdulowork · 2022-11-12T04:41:49Z

lib/Sema/Constraint.cpp

  // Print all type variables as $T0 instead of _ here.
  PrintOptions PO;
  PO.PrintTypesForDebugging = true;

+  Out.indent(firstLineIndent);


Constraints where previously printed unindented, which interfered with folding

abdulowork · 2022-11-12T04:44:42Z

lib/Sema/CSSolver.cpp

      if (result.getKind() == SolutionResult::Success) {
-        llvm::errs() << "\n---Solution---\n";
-        result.getSolution().dump(llvm::errs());


Solutions where printed unindented:

abdulowork · 2022-11-12T04:56:33Z

The log is now easier to traverse with set foldmethod=indent in vim

xedin · 2022-11-12T04:57:31Z

Thank you! I will take a look on monday!

xedin · 2022-11-14T21:57:22Z

include/swift/Sema/ConstraintSystem.h

@@ -3438,6 +3438,10 @@ class ConstraintSystem {
    return findSelectedOverloadFor(calleeLoc);
  }

+  unsigned int getCurrentIndent() const {


We didn't do this before because indent is not really a property of the constraint system. If you want to refactor this, I think it would be reasonable to do what CSStep currently does, where it would return a properly indented getDebugLogger(). Also please separate refactoring changes like this into a commit to make it easier to review functional changes.

The reason I decided to move this logic to the ConstraintSystem was that I noticed a couple of places like this one and this one with similar code that was checking for solverState existence. Since I needed to fetch indentation in numerous other places, it felt like moving this logic to ConstraintSystem would reduce duplication.

I am not sure how to employ getDebugLogger() approach here. There is still a need to get the indentation to pass it on to print constraints.

I can leave the getCurrentIndent() refactoring out of the scope of this pull request and revert to checking for solverState externally where appropriate.

please separate refactoring changes

I split this PR into three commits. If it is too large, I can break it down into several PRs.

Instead of using llvm::errs() directly and setting .indent(solverState ? ...) everywhere we could just ask a constraint system for debug logger which would set appropriate indent internally. It's always possible to change indent when necessary.

I am not sure how to employ getDebugLogger() approach here. There is still a need to get the indentation to pass it on to print constraints.

This would effectively stay the same where getCurrentIndent is kept on the SolverState.

I can leave the getCurrentIndent() refactoring out of the scope of this pull request and revert to checking for solverState externally where appropriate.

I think that would be appropriate, it's effectively NFC refactoring and could be done separately.

it's effectively NFC refactoring and could be done separately

I reverted the getCurrentIndent() refactoring

xedin · 2022-11-14T22:02:03Z

include/swift/Sema/Constraint.h

@@ -891,7 +891,8 @@ class Constraint final : public llvm::ilist_node<Constraint>,
  /// Print constraint placed on type and constraint properties.
  ///
  /// \c skipLocator skips printing of locators.
-  void print(llvm::raw_ostream &Out, SourceManager *sm, unsigned indent = 0,
+  void print(llvm::raw_ostream &Out, SourceManager *sm,
+             unsigned firstLineIndent = 0, unsigned indent = 0,


If I remember correctly the idea here is that the Out is indented when it's passed in that's why indent is used only if the constraint spans multiple lines to be able to indent everything after \n so we need to make sure that this happens correctly instead of adding firstLineIndent here.

That makes sense. I removed firstLineIndent and indented Out where it was appropriate.

xedin · 2022-11-16T17:09:56Z

lib/Sema/CSSolver.cpp

@@ -353,6 +353,10 @@ bool ConstraintSystem::simplify() {
    auto *constraint = &ActiveConstraints.front();
    deactivateConstraint(constraint);

+    auto hasSimplificationResult =


I think isSimplifiable might be a better name here.

xedin

Changes besides ConstraintSystem::getCurrentIdent look good to me, thank you!

xedin

Thank you!

xedin · 2022-11-17T23:06:54Z

@swift-ci please smoke test

xedin · 2022-11-29T17:25:53Z

@swift-ci please smoke test macOS platform

abdulowork commented Nov 12, 2022

View reviewed changes

xedin self-requested a review November 12, 2022 04:57

abdulowork marked this pull request as ready for review November 14, 2022 07:54

xedin requested changes Nov 14, 2022

View reviewed changes

abdulowork force-pushed the improve-type-checker-debugging-indentation branch 3 times, most recently from c9310be to ff706c6 Compare November 15, 2022 17:32

xedin reviewed Nov 16, 2022

View reviewed changes

Fix unbalanced braces in debugging output

3c401a1

abdulowork force-pushed the improve-type-checker-debugging-indentation branch from ff706c6 to d738958 Compare November 17, 2022 15:25

Improve indentation in debugging output

3f36694

abdulowork force-pushed the improve-type-checker-debugging-indentation branch from d738958 to 3f36694 Compare November 17, 2022 15:25

xedin approved these changes Nov 17, 2022

View reviewed changes

xedin merged commit 6b37764 into swiftlang:main Nov 30, 2022

Improve indentation in type checker debugging output #62053

Improve indentation in type checker debugging output #62053

Uh oh!

Conversation

abdulowork commented Nov 11, 2022

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!

abdulowork commented Nov 12, 2022

Uh oh!

xedin commented Nov 12, 2022

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!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Nov 17, 2022

Uh oh!

xedin commented Nov 29, 2022

Uh oh!

Uh oh!