optimizes reconstructor, symtab, and brancher performance #855

gitoleg · 2018-08-09T18:10:18Z

This PR rectifies reconstructor, symtab and brancher with a respect to performance, without addition of a new behavior or breaking of an existing one.

Changes:

reconstructor

Rewrote it and reduced the number of iterations over nodes/edges of cfg.
The main point is how we build the filtered cfg. Previously, we did like the following: take all nodes, add them ony-by-one to an empty cfg. So it was an iteration over all nodes of cfg. Then iterate over all edges, and add each of them if their's destination block is not root.
Also, we did one more iteration over all nodes in find_calls function.

So what I'm proposing here is just to iterate once over the whole cfg to find all roots. and then build the filtered cfg by iterating over the roots and removing their input edges.

symtab

There was a bit inefficient implementation of add_symbol function, so every addition of a symbol called filter on the whole table, although there were enough info to reduce such calls.
Concerning details. Every time we add symbol to the symtab we also remember it's name and address of an entry block. Also we filter memmap from previously added entries,
that have same name and entry block. But, as we remembered the address of the
entry block and the name of the symbol, we can check, if memmap needed to be
filtered without event touching it!

bracnher

Just a small fix that checks if an instruction has jumps at all before the subsequent call of fold_consts that could be heavy for some of the instructions

This PR rectifies reconstructor, symtab and brancher with a respect to performance, without adding new behaviour or breaking of existed one. Rewrote it and reduced a number of iterations over nodes/edges of cfg There was a bit inefficient implementation of `add_symbol` function, so every addition of a symbol led to a filter of the whole table, although there are enough info to reduce such calls. Just a small fix that check if an instruction has jumps at all before subsequent call of `fold_consts` that could be heavy for some of instructions

ivg · 2018-08-09T19:30:05Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+  else init
+
+let find_calls cfg roots =
+  let roots = List.fold ~init:Addr.Set.empty ~f:Set.add roots in


There is Addr.Set.of_list for this.

ivg · 2018-08-09T19:30:09Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+  let addr = Block.addr blk in
+  let term = Block.terminator blk in
+  let init =
+    if Set.mem roots addr || Seq.is_empty (Cfg.Node.inputs blk cfg)


If a block address is already in the set of roots then there is no need to add to the set of roots.
also, to count the indegree of a block use the specialized function Cfg.Node.degree ~dir:In , thus this function
could be defined as simple as

let call_destinations cfg blk = if not Insn.(is call) (Block.terminator blk) then [] else Seq.to_list_rev @@ Seq.filter_map (Cfg.Node.outputs blk cfg) ~f:(fun e -> Option.some_if (Cfg.Edge.label e <> `Fall) (Cfg.Edge.dst e)) let roots_of_blk cfg blk = Addr.Set.of_list @@ List.concat [ if Cfg.Node.degree ~dir:`In = 0 then Block.addr blk else []; call_destinations cfg blk ]

ivg · 2018-08-09T19:37:51Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+
+let find_calls cfg roots =
+  let roots = List.fold ~init:Addr.Set.empty ~f:Set.add roots in
+  Graphlib.depth_first_search (module Cfg)


You don't need DFS for that as you do not need any specific ordering. Just iterate over all nodes. Also, you don't need roots, to check that root is root and add it to roots. This function is simply:

let find_calls cfg roots = Cfg.nodes cfg |> Seq.fold ~init:roots ~f:(fun roots n -> Set.union roots (roots_of_node n))

ivg · 2018-08-09T20:08:18Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+                ~start_tree:(fun n t ->
+                    if Block.equal n entry
+                    then Cfg.Node.insert n t
+                    else return t)) in


So the main difference here is that instead of checking each destination for is it a call or not, we just iterate over all calls and ask for incoming edges of a call. That makes sense, and this is indeed faster, provided that majority of calls are reachable from the whole program CFG.

ivg · 2018-08-10T12:28:03Z

lib/bap_disasm/bap_disasm_symtab.ml

 let add_symbol t (name,entry,cfg) : t =
  let data = name,entry,cfg in
-  let t = remove t data in
+  let t = filter t data in


can you please justify why we can use filter instead of remove here? It's not quite obvious to me.

ivg · 2018-08-10T12:36:00Z

lib/bap_disasm/bap_disasm_symtab.ml

 }

+let filter t ((name,entry,_ ) as fn) =


OK, so from the IRL discussion, the remove function is inefficient since it filters the memory mapping even if the symbol is not in the symtab, the fact that could be checked very fast with the Set membership operation.

However, this is the problem of the remove function, that should be fixed on its side. So, please remove the filter function and add the corresponding check to the remove function (in fact a single test for inclusion in addrs would be sufficient.

ivg

a couple of bad naming choices, but otherwise good to go.
also, you might consider using a hash table instead of set for is_call if it gives a significant boost.

ivg · 2018-08-13T19:18:47Z

lib/bap_disasm/bap_disasm_reconstructor.ml

-                let w = Block.addr (Cfg.Edge.dst e) in
-                Hashtbl.set starts ~key:w ~data:(name w)));
-  starts
+let callees_of_block cfg roots blk =


a bad choice of name, since you're adding the block itself to the resulting set it is no longer a callees_of_block. Suggestion, use entries_of_block since you're effectively collecting the entry nodes.

Also, instead of doing lots of set unions you can pass the set along and use Set.add. It is more efficient.

ivg · 2018-08-13T19:19:28Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+let update_callees cfg roots callees blk =
+  Set.union callees (callees_of_block cfg roots blk)
+
+let find_callees cfg roots =


rename to collect_entries

ivg · 2018-08-13T19:24:15Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+  let callees = find_callees cfg roots in
+  let is_call e = Set.mem callees (Cfg.Edge.dst e) in
+  let rec traverse fng node =
+    let fng = Cfg.Node.insert node fng in


Please, don't use abbreviations that are not common, suggestions,

graph or g or prog for the whole program graph
cfg for the CFG.

Also, not edg, either edge or e.

ivg · 2018-08-13T19:36:17Z

lib/bap_disasm/bap_disasm_reconstructor.ml

-          Symtab.add_symbol syms (name,entry,cfg))
+  let callees = find_callees cfg roots in
+  let is_call e = Set.mem callees (Cfg.Edge.dst e) in
+  let rec traverse fng node =


it is not traverse as this function builds a new graph. The better name reachable or add_reachable or just add.

ivg · 2018-08-13T19:50:00Z

lib/bap_disasm/bap_disasm_reconstructor.ml

+        if is_call edg then fng
+        else
+          let dst = Cfg.Edge.dst edg in
+          let visited = Cfg.Node.mem dst fng in


I think that the following is easier to read

if is_call edge then cfg else let cfg' = Cfg.Edge.insert edge cfg in if Cfg.Node.mem (Cfg.Edge.dst edge) cfg then cfg' else add cfg' node

gitoleg · 2018-08-13T22:03:58Z

so, what about time - the new reconstructor on my machine with our favorite dropbear works 25 seconds less than the old one.

ivg · 2018-08-14T12:36:06Z

relative timing would be much more interesting. 25 seconds out of 3 hours is much different from 25 seconds out of 30 seconds.

gitoleg requested a review from ivg August 9, 2018 19:01

ivg requested changes Aug 10, 2018

View reviewed changes

ivg reviewed Aug 10, 2018

View reviewed changes

ivg changed the title ~~rectifies reconstructor, symtab and brancher~~ optimizes reconstructor, symtab, and brancher performace Aug 10, 2018

updated after review

a2175b7

gitoleg assigned ivg Aug 13, 2018

ivg requested changes Aug 13, 2018

View reviewed changes

ivg changed the title ~~optimizes reconstructor, symtab, and brancher performace~~ optimizes reconstructor, symtab, and brancher performance Aug 13, 2018

ivg assigned gitoleg and unassigned ivg Aug 13, 2018

refactored

b6da46e

gitoleg assigned ivg and unassigned gitoleg Aug 13, 2018

ivg approved these changes Aug 14, 2018

View reviewed changes

ivg merged commit 06b516e into BinaryAnalysisPlatform:master Aug 14, 2018

gitoleg deleted the few-speedups branch October 10, 2018 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimizes reconstructor, symtab, and brancher performance #855

optimizes reconstructor, symtab, and brancher performance #855

gitoleg commented Aug 9, 2018 •

edited

Loading

ivg Aug 9, 2018

ivg Aug 9, 2018

ivg Aug 9, 2018

ivg Aug 9, 2018

ivg Aug 10, 2018

ivg Aug 10, 2018 •

edited

Loading

ivg left a comment

ivg Aug 13, 2018

ivg Aug 13, 2018

ivg Aug 13, 2018

ivg Aug 13, 2018

ivg Aug 13, 2018

gitoleg commented Aug 13, 2018

ivg commented Aug 14, 2018

optimizes reconstructor, symtab, and brancher performance #855

optimizes reconstructor, symtab, and brancher performance #855

Conversation

gitoleg commented Aug 9, 2018 • edited Loading

reconstructor

symtab

bracnher

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

ivg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitoleg commented Aug 13, 2018

ivg commented Aug 14, 2018

gitoleg commented Aug 9, 2018 •

edited

Loading

ivg Aug 10, 2018 •

edited

Loading