Performance improvement refactors

Author: shreyasbharath

.vscode/launch.json

@@ -24,7 +24,7 @@
       "pathToBundler": "C:/tools/ruby25/bin/bundle.bat",
       "pathToRDebugIDE": "C:/tools/ruby25/bin/rdebug-ide.bat",
       "showDebuggerOutput": true,
-      "args": ["generate_graph", "--root_dir", "../protobuf/src", "--output_file", "deps.dot"]
+      "args": ["visualise", "--root_dir", "../protobuf/src"]
     },
     {
       "name": "Listen for rdebug-ide",

TODO.md

@@ -15,6 +15,7 @@
   - [ ] Work with any type of include relative includes ('blah.h') vs absolute includes ('/path/blah.h') vs relative with path includes ('blah/blah.h')
   - [ ] Look at https://github.com/Sarcasm/compdb to see if it can generate dependencies
 - [ ] Parallelise dependency scanning as much as possible to get the best possible performance
+  - [ ] Use a threadpool? Look at reference implementations
 - [ ] Open up the graph automatically after generating an individual graph
 - [ ] Work with any sort of project structure
   - [ ] Header only projects

exe/cpp_dependency_graph

@@ -25,11 +25,11 @@
 require 'docopt'
 require 'cpp_dependency_graph'
 
-# require 'ruby-prof'
+require 'ruby-prof'
 
 include CppDependencyGraph
 
-# RubyProf.start
+RubyProf.start
 
 doc = <<DOCOPT
   Cpp Dependency Graph
@@ -82,6 +82,6 @@ rescue Docopt::Exit => e
   puts e.message
 end
 
-# result = RubyProf.stop
-# printer = RubyProf::CallTreePrinter.new(result)
-# printer.print(path: '.', profile: 'cpp_dependency_graph')
+result = RubyProf.stop
+printer = RubyProf::CallTreePrinter.new(result)
+printer.print(path: '.', profile: 'cpp_dependency_graph')

lib/cpp_dependency_graph/cycle_detector.rb

@@ -5,13 +5,11 @@
 # Detects cycles between components
 class CycleDetector
   def initialize(component_links)
-    @cyclic_links = {}
-    component_links.each do |source, links|
-                      links.each do |target|
-                        links_of_target = component_links[target]
-                        @cyclic_links[CyclicLink.new(source, target)] = true if links_of_target.include?(source)
+    @cyclic_links = component_links.flat_map do |source, links|
+                      links.select { |target| component_links[target].include?(source) }.map do |target|
+                        [CyclicLink.new(source, target), true]
                       end
-                    end
+                    end.to_h
   end
 
   def cyclic?(source, target)
@@ -20,6 +18,6 @@ def cyclic?(source, target)
   end
 
   def to_s
-    @cyclic_links.each { |k, _| puts k }
+    @cyclic_links.keys
   end
 end

lib/cpp_dependency_graph/dependency_graph.rb

@@ -24,24 +24,26 @@ def all_cyclic_links
 
   def component_links(name)
     return {} unless all_component_links.key?(name)
-    incoming_links(name).merge(outgoing_links(name))
+    links = incoming_links(name)
+    links.merge!(outgoing_links(name))
+    links
   end
 
   private
 
   def build_hash_component_links
-    raw_links = @project.source_components.map { |c| [c.name, @project.dependencies(c)] }.to_h
-    cycle_detector = CycleDetector.new(raw_links)
+    raw_links = @project.source_components.values.map { |c| [c.name, @project.dependencies(c)] }.to_h
+    @cycle_detector ||= CycleDetector.new(raw_links)
     component_links = raw_links.map do |source, links|
-                        c_links = links.map { |target| ComponentLink.new(source, target, cycle_detector.cyclic?(source, target)) }
+                        c_links = links.map { |target| ComponentLink.new(source, target, @cycle_detector.cyclic?(source, target)) }
                         [source, c_links]
                       end.to_h
     component_links
   end
 
   def build_cyclic_links
-    cyclic_links = all_component_links.select { |_, links| links.any? { |link| link.cyclic? } }
-    cyclic_links.each { |_, links| links.select! { |link| link.cyclic? } }
+    cyclic_links = all_component_links.select { |_, links| links.any?(&:cyclic?) }
+    cyclic_links.each_value { |links| links.select! { |link| link.cyclic? } }
   end
 
   def outgoing_links(name)
@@ -50,9 +52,9 @@ def outgoing_links(name)
 
   def incoming_links(target)
     incoming_c_links = all_component_links.select { |c, c_links| c_links.any? { |link| link.target == target } }
-    incoming_c_links.map do |c_name, _|
-      link = ComponentLink.new(c_name, target, false)
-      [c_name, [link]]
+    incoming_c_links.map do |source, _|
+      link = ComponentLink.new(source, target, @cycle_detector.cyclic?(source, target))
+      [source, [link]]
     end.to_h
   end
 end

lib/cpp_dependency_graph/graph_visualiser.rb

@@ -8,7 +8,7 @@ def generate_dot_file(deps, file)
     @g = Graphviz::Graph.new(name = 'dependency_graph')
 
     node_names = deps.flat_map do |_, links|
-                   links.map { |link| [link.source, link.target] }.flatten
+                   links.map { |link| [link.source, link.target] }
                  end.uniq
     nodes = node_names.map { |name| [name, create_node(name)] }.to_h
 
@@ -25,7 +25,7 @@ def generate_dot_file(deps, file)
 
   def generate_html_file(deps, file)
     node_names = deps.flat_map do |_, links|
-                   links.map { |link| [link.source, link.target] }.flatten
+                   links.map { |link| [link.source, link.target] }
                  end.uniq
     nodes = node_names.map { |name| { name: name } }
 

lib/cpp_dependency_graph/include_dependency_graph.rb

@@ -13,12 +13,13 @@ def initialize(project)
   def include_links(component_name)
     component = @project.source_component(component_name)
     source_files = component.source_files
+    external_includes = @project.external_includes(component)
     source_files.map do |file|
-      class_name = file.basename
       # TODO: Very inefficient
-      internal_includes = file.includes.reject { |inc| @project.external_includes(component).any?(inc) }
-      include_links = internal_includes.map { |inc| ComponentLink.new(class_name, inc, false) }
-      [class_name, include_links]
+      source = file.basename
+      internal_includes = file.includes.reject { |inc| external_includes.any?(inc) }
+      include_links = internal_includes.map { |inc| ComponentLink.new(source, inc, false) }
+      [source, include_links]
     end.to_h
   end
 end

lib/cpp_dependency_graph/include_to_component_resolver.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require_relative 'source_component'
+
+# Resolves a given include to a source component
+class IncludeToComponentResolver
+  def initialize(components)
+    @components = components
+    @component_external_include_cache = {}
+    @component_include_map_cache = {}
+  end
+
+  def external_includes(component)
+    unless @component_external_include_cache.key?(component)
+      @component_external_include_cache[component] = external_includes_private(component)
+    end
+    @component_external_include_cache[component]
+  end
+
+  def component_for_include(include)
+    return '' unless source_files.key?(include)
+    unless @component_include_map_cache.key?(include)
+      @component_include_map_cache[include] = component_for_include_private(include)
+    end
+    @component_include_map_cache[include]
+  end
+
+  private
+
+  def external_includes_private(component)
+    include_components = component.includes.map { |inc| [inc, component_for_include(inc)] }.to_h
+    external_include_components = include_components.delete_if { |_, c| c == component.name }
+    external_include_components.keys
+  end
+
+  def component_for_include_private(include)
+    header_file = source_files[include]
+    implementation_files = implementation_files(header_file)
+    return header_file.parent_component if implementation_files.empty?
+    implementation_files[0].parent_component
+  end
+
+  def implementation_files(header_file)
+    implementation_files = []
+    source_files.each_value do |file|
+      implementation_files.push(file) if file.basename_no_extension == header_file.basename_no_extension
+    end
+    implementation_files.reject! { |file| file.basename == header_file.basename }
+  end
+
+  def source_files
+    @source_files ||= build_source_file_map
+  end
+
+  def build_source_file_map
+    # TODO SourceComponent should return a hash for source files which can be merged here
+    source_files = @components.values.flat_map(&:source_files)
+    source_files.map { |s| [s.basename, s] }.to_h
+  end
+end

lib/cpp_dependency_graph/project.rb

@@ -2,62 +2,44 @@
 
 require 'find'
 
+require_relative 'include_to_component_resolver'
 require_relative 'source_component'
 
 # Parses all components of a project
 class Project
   def initialize(path)
     @path = path
+    @include_resolver = IncludeToComponentResolver.new(source_components)
   end
 
   def source_components
     @source_components ||= build_source_components
   end
 
   def source_component(name)
-    source_components.detect { |c| c.name == name }
+    return SourceComponent.new('NULL') unless source_components.key?(name)
+    source_components[name]
   end
 
   def dependencies(component)
-    external_includes(component).map { |include| component_for_include(include) }.reject(&:empty?).uniq
+    # TODO: This is repeating the same work twice! component_for_include is called when calling external_includes
+    external_includes(component).map { |include| @include_resolver.component_for_include(include) }.reject(&:empty?).uniq
   end
 
   def external_includes(component)
-    filter_internal_includes(component)
+    @include_resolver.external_includes(component)
   end
 
   private
 
   def build_source_components
     # TODO: Dealing with source components with same dir name?
     dirs = fetch_all_dirs(@path)
-    source_components = dirs.map { |dir| SourceComponent.new(dir) }
-    source_components.reject { |c| c.source_files.size.zero? }
-  end
-
-  def filter_internal_includes(component)
-    # TODO: This is super inefficient, refactor it
-    source_file_basenames = component.source_files.map(&:basename)
-    include_components = component.includes.map { |inc| [inc, component_for_include(inc)] }.to_h
-    filter = include_components.reject { |_, c| c == component.name }
-    filter.keys
-  end
-
-  def component_for_include(include)
-    header_file = source_files.find { |file| file.basename == include }
-    parent_component(header_file)
-  end
-
-  def source_files
-    @source_files ||= source_components.flat_map(&:source_files)
-  end
-
-  def parent_component(header_file)
-    return '' if header_file.nil?
-    files = source_files.select { |file| file.basename_no_extension == header_file.basename_no_extension }
-    corresponding_files = files.reject { |file| file.basename == header_file.basename }
-    return header_file.parent_component if corresponding_files.size == 0
-    corresponding_files[0].parent_component
+    components = dirs.map do |dir|
+                   c = SourceComponent.new(dir)
+                   [c.name, c]
+                 end.to_h
+    components.delete_if { |k, v| v.source_files.size.zero? }
   end
 
   def fetch_all_dirs(source_dir)

lib/cpp_dependency_graph/source_component.rb

@@ -31,7 +31,6 @@ def loc
 
   def parse_source_files(extensions)
     path = File.join(@path, File::SEPARATOR) + '*' + extensions
-    files = Dir.glob(path).select { |e| File.file?(e) }
-    files.map { |file| SourceFile.new(file) }
+    Dir.glob(path).map { |e| SourceFile.new(e) if File.file?(e) }.compact
   end
 end

lib/cpp_dependency_graph/source_file.rb

@@ -41,8 +41,9 @@ def all_includes
   end
 
   def scan_includes
-    # file_contents.scan(/#include "([^"]+)"/).uniq.flatten
-    file_contents.scan(/#include ["|<](.+)["|>]/).uniq.flatten   # TODO: use compiler lib to scan includes? llvm/clang?
+    includes = file_contents.scan(/#include ["|<](.+)["|>]/) # TODO: use compiler lib to scan includes? llvm/clang?
+    includes.uniq!
+    includes.flatten
   end
 
   def file_contents

lib/cpp_dependency_graph/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module CppDependencyGraph
-  VERSION = '0.1.1'
+  VERSION = '0.1.2'
 end

spec/test/dependency_graph_spec.rb

@@ -19,9 +19,13 @@
     expect(dependency_graph.all_component_links).to eq(expected_links)
   end
 
+  it 'returns empty links for an unknown component of a project' do
+    expect(dependency_graph.component_links('Blah').empty?).to eq(true)
+  end
+
   it 'returns links for a specified component of a project' do
     expected_links = {}
-    expected_links['UI'] = [ComponentLink.new('UI', 'Engine', false)]   # TODO
+    expected_links['UI'] = [ComponentLink.new('UI', 'Engine', true)]
     expected_links['Engine'] = [ComponentLink.new('Engine', 'Framework', false),
                       ComponentLink.new('Engine', 'UI', true),
                       ComponentLink.new('Engine', 'DataAccess', false)]

spec/test/include_dependency_graph_spec.rb

@@ -0,0 +1,21 @@
+require 'cpp_dependency_graph/include_dependency_graph'
+require 'cpp_dependency_graph/component_link'
+
+RSpec.describe IncludeDependencyGraph do
+  let(:project) { Project.new('spec/test/example_project') }
+  let(:include_dependency_graph) { IncludeDependencyGraph.new(project) }
+
+  it 'returns empty hash for an unknown component' do
+    expect(include_dependency_graph.include_links('Unknown').empty?).to eq(true)
+  end
+
+  it 'returns include links for a specified component' do
+    expected_links = {}
+    expected_links['OldEngine.h'] = []
+    expected_links['Engine.h'] = [ComponentLink.new('Engine.h', 'Framework/framework.h', false),
+                                  ComponentLink.new('Engine.h', 'UI/Display.h', false)]
+    expected_links['Engine.cpp'] = [ComponentLink.new('Engine.cpp', 'DataAccess/DA.h', false),
+                                    ComponentLink.new('Engine.cpp', 'Engine.h', false)]
+    expect(include_dependency_graph.include_links('Engine')).to eq(expected_links)
+  end
+end

spec/test/project_spec.rb

@@ -2,13 +2,14 @@
 
 RSpec.describe Project do
   it 'parses all source components' do
-    component_names = Project.new('spec/test/example_project').source_components.map(&:name).sort
+    component_names = Project.new('spec/test/example_project').source_components.values.map(&:name).sort
     expect(component_names).to eq(['DataAccess', 'Engine', 'Framework', 'System', 'UI', 'main'])
   end
 
-  it 'returns nil component if case does not match' do
+  it 'returns null component if case does not match' do
     component = Project.new('spec/test/example_project').source_component('enGine')
-    expect(component).to eq(nil)
+    expect(component.name).to eq('NULL')
+    expect(component.source_files.size).to eq(0)
   end
 
   it 'returns dependencies of component' do