@Performance critical checker: compute the expensive call stack lazily when reporting errors

Summary:public
Before this diff, the checker was collecting in a bottom-up fashion all possible call trees from `PerforamanceCritical`-annotated methods to `Expensive`-annotated ones. With this diff, we just collect the names of the direct transitively expensive callees and compute the expensive call stacks when reporting errors only.

Reviewed By: sblackshear

Differential Revision: D2938635

fb-gh-sync-id: dcdd13c
shipit-source-id: dcdd13c

infer/src/backend/callTree.ml

@@ -1,27 +0,0 @@
-(*
- * Copyright (c) 2015 - present Facebook, Inc.
- * All rights reserved.
- *
- * This source code is licensed under the BSD style license found in the
- * LICENSE file in the root directory of this source tree. An additional grant
- * of patent rights can be found in the PATENTS file in the same directory.
- *)
-
-
-module F = Format
-
-type call = Procname.t * Location.t
-
-type t =
-  | Direct of call
-  | Indirect of call * t list
-
-
-let pp fmt tree =
-  let rec loop stack = function
-    | Direct (pname, _) ->
-        F.fprintf fmt "%s -> %s" stack (Procname.to_string pname)
-    | Indirect ((pname, _), l) ->
-        let stack' = stack ^ " -> " ^ (Procname.to_string pname) in
-        IList.iter (loop stack') l in
-  loop "@" tree

infer/src/backend/callTree.mli

@@ -1,18 +0,0 @@
-(*
- * Copyright (c) 2015 - present Facebook, Inc.
- * All rights reserved.
- *
- * This source code is licensed under the BSD style license found in the
- * LICENSE file in the root directory of this source tree. An additional grant
- * of patent rights can be found in the PATENTS file in the same directory.
- *)
-
-type call = Procname.t * Location.t
-
-(** data-structure to represent call stacks in a compact tree form *)
-type t =
-  | Direct of call
-  | Indirect of call * t list
-
-(** print the list of call stacks in the tree *)
-val pp : Format.formatter -> t -> unit

infer/src/backend/specs.ml

@@ -299,12 +299,14 @@ type phase = FOOTPRINT | RE_EXECUTION
 
 type dependency_map_t = int Procname.Map.t
 
+type call = Procname.t * Location.t
+
 (** Payload: results of some analysis *)
 type payload =
   {
     preposts : NormSpec.t list option; (** list of specs *)
     typestate : unit TypeState.t option; (** final typestate *)
-    calls:  CallTree.t list option;
+    calls: call list option;
   }
 
 type summary =

infer/src/backend/specs.mli

@@ -114,12 +114,15 @@ type phase = FOOTPRINT | RE_EXECUTION
 
 type dependency_map_t = int Procname.Map.t
 
+(** Type for calls consiting in the name of the callee and the location of the call *)
+type call = Procname.t * Location.t
+
 (** Payload: results of some analysis *)
 type payload =
   {
     preposts : NormSpec.t list option; (** list of specs *)
     typestate : unit TypeState.t option; (** final typestate *)
-    calls:  CallTree.t list option; (** list of call tree *)
+    calls:  call list option; (** list of call tree *)
   }
 
 (** Procedure summary *)

infer/src/checkers/performanceCritical.ml

@@ -78,39 +78,42 @@ let method_is_expensive tenv pname =
   || check_method Annotations.ia_is_expensive pname
 
 
-let lookup_call_trees pname =
+let lookup_expensive_calls pname =
   match Specs.get_summary pname with
   | None -> []
   | Some summary ->
       begin
         match summary.Specs.payload.Specs.calls with
-        | Some tree -> tree
+        | Some calls -> calls
         | None -> []
       end
 
 
+let method_calls_expensive tenv pname =
+  method_is_expensive tenv pname
+  || lookup_expensive_calls pname <> []
+
+
 let lookup_location pname =
   match Specs.get_summary pname with
   | None -> Location.dummy
   | Some summary -> summary.Specs.attributes.ProcAttributes.loc
 
 
-let collect_expensive_call tenv caller_pdesc checked_pnames call_trees (pname, _) =
-  if Procname.Set.mem pname !checked_pnames then call_trees
+let collect_expensive_call tenv caller_pdesc checked_pnames call_list (pname, _) =
+  if Procname.Set.mem pname !checked_pnames then call_list
   else
     begin
       Ondemand.do_analysis caller_pdesc pname;
       checked_pnames := Procname.Set.add pname !checked_pnames;
       let call_loc = lookup_location pname in
-      if method_is_expensive tenv pname then
-        (CallTree.Direct (pname, call_loc)) :: call_trees
+      if method_calls_expensive tenv pname then
+        (pname, call_loc) :: call_list
       else
-        match lookup_call_trees pname with
-        | [] -> call_trees
-        | calls -> (CallTree.Indirect ((pname, call_loc), calls)) :: call_trees
+        call_list
     end
 
-let update_summary call_trees pname =
+let update_summary calls pname =
   match Specs.get_summary pname with
   | None -> ()
   | Some summary ->
@@ -118,12 +121,12 @@ let update_summary call_trees pname =
         { summary with
           Specs.payload =
             { summary.Specs.payload with
-              Specs.calls = Some call_trees; }
+              Specs.calls = Some calls; }
         } in
       Specs.add_summary pname updated_summary
 
 
-let report_expensive_calls pname pdesc loc call_trees =
+let report_expensive_calls tenv pname pdesc loc calls =
   let string_of_pname = Procname.to_simplified_string ~withclass:true in
   let update_trace trace loc =
     if Location.equal loc Location.dummy then trace
@@ -135,11 +138,10 @@ let report_expensive_calls pname pdesc loc call_trees =
         lt_node_tags = [];
       } in
       trace_elem :: trace in
-  let rec report_call_tree (trace, stack_str) call_tree =
-    match call_tree with
-    | CallTree.Direct (expensive_pname, callee_loc) ->
+  let rec report_expensive_call visited_pnames (trace, stack_str) (callee_pname, callee_loc) =
+    if method_is_expensive tenv callee_pname then
       let final_trace = IList.rev (update_trace trace callee_loc) in
-        let exp_pname_str = string_of_pname expensive_pname in
+      let exp_pname_str = string_of_pname callee_pname in
       let description =
         Printf.sprintf
           "Method `%s` annotated with `@%s` calls `%s%s` where `%s` is annotated with `@%s`"
@@ -152,13 +154,20 @@ let report_expensive_calls pname pdesc loc call_trees =
       let exn =
         Exceptions.Checkers (calls_expensive_method, Localise.verbatim_desc description) in
       Reporting.log_error pname ~loc: (Some loc) ~ltr: (Some final_trace) exn
-    | CallTree.Indirect ((callee_pname, callee_loc), sub_trees) ->
+    else
+      let next_calls = lookup_expensive_calls callee_pname in
       let callee_pname_str = string_of_pname callee_pname in
       let new_stack_str = stack_str ^ callee_pname_str ^ " -> " in
       let new_trace = update_trace trace callee_loc in
-        IList.iter (report_call_tree (new_trace, new_stack_str)) sub_trees in
+      let unseen_pnames, updated_visited =
+        IList.fold_left
+          (fun (accu, set) (p, loc) ->
+             if Procname.Set.mem p set then (accu, set)
+             else ((p, loc) :: accu, Procname.Set.add p set))
+          ([], visited_pnames) next_calls in
+      IList.iter (report_expensive_call updated_visited (new_trace, new_stack_str)) unseen_pnames in
   let start_trace = update_trace [] (Cfg.Procdesc.get_loc pdesc) in
-  IList.iter (report_call_tree (start_trace, "")) call_trees
+  IList.iter (report_expensive_call Procname.Set.empty (start_trace, "")) calls
 
 
 let check_one_procedure tenv pname pdesc =
@@ -186,18 +195,15 @@ let check_one_procedure tenv pname pdesc =
     PatternMatch.proc_iter_overridden_methods
       check_expensive_subtyping_rules tenv pname;
 
-  let expensive_call_trees =
+  let expensive_calls =
     let checked_pnames = ref Procname.Set.empty in
     Cfg.Procdesc.fold_calls
       (collect_expensive_call tenv pdesc checked_pnames) [] pdesc in
 
-  update_summary expensive_call_trees pname;
+  update_summary expensive_calls pname;
 
-  match expensive_call_trees with
-  | [] -> ()
-  | call_trees when performance_critical ->
-      report_expensive_calls pname pdesc loc call_trees
-  | _ -> ()
+  if performance_critical then
+    report_expensive_calls tenv pname pdesc loc expensive_calls
 
 
 let callback_performance_checker { Callbacks.proc_desc; proc_name; get_proc_desc; tenv } =

infer/tests/codetoanalyze/java/checkers/ExpensiveCallExample.java

@@ -45,7 +45,10 @@ public class ExpensiveCallExample implements AnnotatedInterface {
   void nonExpensiveMethod() {}
 
   @Expensive
-  void expensiveMethod() {}
+  void expensiveMethod() {
+    // The checker should still report the expensive call stack despite the call cycle
+    methodWrapper();
+  }
 
   void methodWrapper() {
     expensiveMethod();