[SIL Optimization] Add a mandatory pass for optimizing the new os log APIs based on string interpolation. #24336

ravikandhadai · 2019-04-27T03:38:59Z

This PR adds a mandatory pass for optimizing the new os_log APIs based on string interpolation. The new os_log APIs are not publicly visible and are currently in stdlib/private. The goal of this optimization pass is to extract the format string and other compile-time constants from the string interpolation and make them literals. For this purpose, this pass uses the constant evaluator and folds string- and integer-valued constants inferred by the evaluator. This PR also include compile-time tests for the optimization pass, which are an addition to the existing runtime tests for the new os log APIs.

The PR consists of three commits, the latest commit has the relevant changes. The earlier two commits corresponds to other PRs that have not yet landed: refactoring of os log library implementation and skip functionality of constant evaluation.

ravikandhadai · 2019-04-27T03:43:23Z

@swift-ci Please test

swift-ci · 2019-04-27T05:23:17Z

Build failed
Swift Test OS X Platform
Git Sha - 73ba475c6c147c016edb150de3dc4d4036ee8fc9

ravikandhadai · 2019-04-27T17:32:22Z

@swift-ci Please test macOS Platform

swift-ci · 2019-04-27T18:17:29Z

Build failed
Swift Test OS X Platform
Git Sha - 73ba475c6c147c016edb150de3dc4d4036ee8fc9

ravikandhadai · 2019-04-28T18:10:31Z

@swift-ci Please smoke test macOS Platform

ravikandhadai · 2019-04-29T18:09:35Z

@swift-ci Please test macOS Platform

eeckstein

basically LGTM!
See my comments inline.

eeckstein · 2019-04-29T21:51:39Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  /// failed.
+  Optional<SymbolicValue> collectConstants(
+      SILBasicBlock::iterator first, SILBasicBlock::iterator last,
+      llvm::SmallMapVector<SingleValueInstruction *, SymbolicValue, 4>


It would make sense to define a typedef for llvm::SmallMapVector<SingleValueInstruction *, SymbolicValue, 4>.

eeckstein · 2019-04-29T21:54:02Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  // the function body. This information is needed for constant folding string
+  // literals and recording it will avoid reconstructing it.
+  SILFunction *stringInitIntrinsic = nullptr;
+  Optional<SILType> stringMetatype = None;


No need for an Optional here. SILType has a null-value:

SILType stringMetatype = SILType();

eeckstein · 2019-04-29T21:56:07Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+
+/// Find the SILBasicBlock::iterator for a given instruction.
+static SILBasicBlock::iterator
+findIteratorForInstruction(SILInstruction *keyInst) {


Why not just use keyInst->getIterator()?

eeckstein · 2019-04-29T21:59:08Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+static SILValue findOSLogMessageArgument(ApplyInst *oslogCall) {
+  ASTContext &astContext = oslogCall->getFunction()->getASTContext();
+
+  for (auto argument : oslogCall->getArguments()) {


Can this be any argument?
If it's defined to be e.g. the first argument, I think you should just check the first argument and not try to find an argument with the right type.

There are two functions that use this type, one is the log function and the other is a mock test helper that does some runtime checks instead of logging and is used only in tests. The mock function has this as the first argument, whereas the log function has this as the second argument. I am also expecting that the log function may have to change/shuffle its arguments as it has to be API compatible with the server-side logging APIs. Just to be robust against these changes, I am searching for the argument by identifier.

ok. I suggest that you add a comment describing this

eeckstein · 2019-04-29T21:59:27Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+      return argument;
+    }
+  }
+  llvm_unreachable("No argument of type OSLogMessage in os log call");


llvm_unreachable?
Shouldn't this be a regular error message?

eeckstein · 2019-04-29T22:01:48Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+}
+
+/// A utility function for inlining the apply instruction into the caller.
+/// TODO: should this function be moved to the SILInliner utility?


Yes, that would make sense. AFAIK, the code is the same in the MandatoryInliner and in the PerformanceInliner

@eeckstein I created a new PR to move this to the SILInliner and modified PerformanceInliner to use this utility.

eeckstein · 2019-04-29T22:15:57Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  if (errorDetected) {
+    return;
+  }
+  // The necessary string information must have been initialized here.


Is it always guaranteed that during collectConstants we see a call to makeUTF8?

Yeah, that's right. We have an empty string literal used in the initializer of the this struct and will be seen during traversal.

eeckstein · 2019-04-29T22:40:22Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  if (shouldAttemptEvaluation(inst, stepEval)) {
+    return stepEval.tryEvaluateOrElseMakeEffectsNonConstant(instI);
+  }
+  return stepEval.skipByMakingEffectsNonConstant(instI);


I assume this will mark the alloc_stack of the string as "unknown" if this is an unknown function which (potentially) modifies the string in the alloc_stack

Yes. It will mark the content of the address returned by alloc_stack as unknown. The address itself is a valid symbolic value, but it contents will be unknown.

ravikandhadai · 2019-05-07T01:34:21Z

@eeckstein Thanks for the comments. I have pushed a commit that addresses them.

ravikandhadai · 2019-05-07T01:34:38Z

@swift-ci Please test

swift-ci · 2019-05-07T01:36:41Z

Build failed
Swift Test Linux Platform
Git Sha - 73ba475c6c147c016edb150de3dc4d4036ee8fc9

swift-ci · 2019-05-07T01:38:53Z

Build failed
Swift Test OS X Platform
Git Sha - 73ba475c6c147c016edb150de3dc4d4036ee8fc9

atrick

Overall the implementation is very well encapsulated into functions and commented.
Some of my feedback is about style and code organization, which never needs to not hold up a PR.
(I suspect that feedback would apply to plenty of existing SIL passes).
If you make one round of answering these review questions (via code comments wherever possible) and do whatever code reorganization you think is appropriate, then you can follow up with other changes later.

atrick · 2019-05-09T05:50:23Z

include/swift/SILOptimizer/PassManager/Passes.def

@@ -307,6 +307,8 @@ PASS(SerializeSILPass, "serialize-sil",
     "Utility pass. Serializes the current SILModule")
 PASS(YieldOnceCheck, "yield-once-check",
    "Check correct usage of yields in yield-once coroutines")
+PASS(OSLogOptimization, "os-log-mandatory-optimization",


Is there potentially going to be an "os-log-optimization" later that is not mandatory?

No there won't be any non-mandatory optimization. I will drop "mandatory" from the name.

atrick · 2019-05-09T05:53:08Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//


I would like to see a file-level doc comment summarizing the purpose of the pass. It also good to put any pass-dependencies or SIL stage prerequisites here.

atrick · 2019-05-09T05:57:32Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors


The past two years happened.

atrick · 2019-05-10T18:45:56Z

test/SILOptimizer/OSLogPrototypeCompileDiagnostics.swift

+
+  // FIXME: the following two tests should produce diagnostics and are not
+  // valid uses of the log APIs. Only string interpolation literal should be
+  // passed to log methods.


Does this FIXME mean that these let formatString examples won't be optimized?

Yes, but actually, this case should even produce a compile-time error. The error should be on "h.log(level: .debug, formatString)", which expects the second parameter to be a string interpolation. let formatString = ... is not by itself an error, it will not be optimized as the optimization is driven by the calls to the log function.

atrick · 2019-05-10T18:46:38Z

test/SILOptimizer/OSLogPrototypeCompileTest.swift

+// Tests for the OSLogOptimization pass that performs compile-time analysis
+// and optimization of the new os log prototype APIs. The tests here check
+// whether specific compile-time constants such as the format string,
+// the size of the byte buffer etc. are literals after the mandatory pipeline.


Nice test cases!

atrick · 2019-05-10T21:38:53Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  // Inline the log call into the caller and find the last instruction of the
+  // log call.
+  SILOptFunctionBuilder funcBuilder(*this);
+  FullApplySite oslogApplySite(oslogCall);


You don't need to explicitly construct FullApplySite. Just pass osLogCall.

atrick · 2019-05-10T22:29:09Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+
+  // An allocator to store symbolic values constructed during interpretation.
+  SymbolicValueBumpAllocator allocator;
+  ConstExprStepEvaluator constantEvaluator(allocator, firstInst->getFunction());


FWIW, every time I've tried to cheat and avoid defining a new class for the per-invocation state of the pass, I've regretted it. I always end keeping pass state in a separate class. Usually, the only thing the SILFunctionTransform does is grab any analyses that are needed and construct the pass object. e.g. SILGenCleanup.cpp and CopyPropagation.cpp.

In your case, the only state is for the constant evaluator, so I would have defined a class for the "argument evaluator" that explicitly ties the lifetime of the constantMap and constantEvaluator to the bump allocator. The fact that lifetimes are dependent is not obvious to the reader when they just happen to be defined in a certain order on the stack.

[EDIT] it turns out that there's hidden state here that lives for the duration of the compilation unit. I would have put the string intrinsic metadata cache in a separate object that other pass objects can refer back to, rather than letting that detail determine the rest of the code organization.

That's a great suggestion. As you mention in the "edit", there is a "string intrinsic metadata cache", but its lifetime can also be linked to the constant evaluator and constantMap as it is something that will be available in every invocation. So I can also move that state to a new "argument evaluator" class.

atrick · 2019-05-10T22:46:17Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  // the function body. This information is needed for constant folding string
+  // literals and recording it will avoid reconstructing it.
+  SILFunction *stringInitIntrinsic = nullptr;
+  SILType stringMetatype = SILType();


I was nearly done with this review before I realized that OSLogOptimization had any state. Please, please define all data members before any instance methods!

If you want to "colocate" the data members and extractStringInfoFromInstruction, then move them to a separate class.

atrick · 2019-05-10T22:58:10Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+
+    // Here we have either a String decl or a Stdlib integer decl or a
+    // builtin integer type.
+    constantMap.insert({singleValInst, constantVal.getValue()});


Why do you need to insert every symbolic constant value into another map? The lifetime of the constantEvaluator is the same as this map, so you can just reuse the map it already has. For example, to determine which instructions are String/Int, you can just keep a SmallDenseSet<SingleValueInstruction *> and reiterate over the instruction range in substituteConstants. Or what would be really great is if you just apply the above filter inside substituteConstants and completely eliminate constantMap!

Thats a good suggestion. You are right that constantEvaluator has the map. The filter can also be moved to substituteConstants and we can eliminate the constantMap state.

atrick · 2019-05-10T23:12:51Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  assert(interpolationStruct);
+
+  auto propertyDecls = interpolationStruct->getStoredProperties();
+  auto propertyValues = osLogInterpolationValue.getAggregateValue();


Overuse of auto makes the code harder for me to follow. I only use auto in three cases:
(1) when the type name is already spelled (or mostly spelled) on the same line e.g. cast, getLoc, getFunction, getModule...
(2) when defining iterators that have long dependent type names
(3) when it is otherwise brutally obvious

Sure, will fix this.

the new os log APIs based on string interpolation.

ravikandhadai · 2019-05-14T02:42:04Z

Fixing @atrick comments

ravikandhadai · 2019-05-14T02:42:16Z

@swift-ci Please test

ravikandhadai requested review from atrick, devincoughlin and eeckstein April 27, 2019 03:38

ravikandhadai mentioned this pull request Apr 27, 2019

[os_log][stdlib-private] Refactor new os log APIs based on string interpolation for enabling optimizations. #24335

Merged

eeckstein reviewed Apr 29, 2019

View reviewed changes

ravikandhadai force-pushed the oslog-optimization branch from 73ba475 to b7a8dce Compare May 3, 2019 21:55

ravikandhadai mentioned this pull request May 3, 2019

[SIL Inlining] Extract out a reusable code snippet in PerformanceInliner into a utility function in SILInliner. #24488

Merged

ravikandhadai force-pushed the oslog-optimization branch from b7a8dce to a4822fd Compare May 7, 2019 01:32

atrick approved these changes May 10, 2019

View reviewed changes

[SIL Optimization] Add a mandatory optimization pass for optimizing

b7b4662

the new os log APIs based on string interpolation.

ravikandhadai force-pushed the oslog-optimization branch from a4822fd to b7b4662 Compare May 14, 2019 02:41

ravikandhadai merged commit a6ff7bd into swiftlang:master May 14, 2019

This was referenced May 14, 2019

Revert "[SIL Optimization] Add a mandatory pass for optimizing the new os log APIs based on string interpolation." #24783

Merged

[SIL Optimization] Add a mandatory optimization pass for optimizing the new os log APIs based on string interpolation. #24796

Merged

[SIL Optimization] Add a mandatory pass for optimizing the new os log APIs based on string interpolation. #24336

[SIL Optimization] Add a mandatory pass for optimizing the new os log APIs based on string interpolation. #24336

Uh oh!

Conversation

ravikandhadai commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravikandhadai commented Apr 27, 2019

Uh oh!

swift-ci commented Apr 27, 2019

Uh oh!

ravikandhadai commented Apr 27, 2019

Uh oh!

swift-ci commented Apr 27, 2019

Uh oh!

ravikandhadai commented Apr 28, 2019

Uh oh!

ravikandhadai commented Apr 29, 2019

Uh oh!

eeckstein 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!

ravikandhadai commented May 7, 2019

Uh oh!

ravikandhadai commented May 7, 2019

Uh oh!

swift-ci commented May 7, 2019

Uh oh!

swift-ci commented May 7, 2019

Uh oh!

atrick 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!

ravikandhadai May 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ravikandhadai commented Apr 27, 2019 •

edited

Loading

ravikandhadai May 11, 2019 •

edited

Loading