From 36b29a966ebfcdf9c57dc3358c0dc6fa08a47ef0 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Mon, 20 Mar 2017 05:25:45 -0700 Subject: [PATCH] Add roadmap feature to js-dep-visualizer. The js-dep-visualizer tool now attempts to find a set of edges to remove from a call graph that would reduce it to having only trivial mutual dependencies, and it produces a roadmap of the changes that need to happen. If the tool can't reduce the graph all the way, it still produces a DOT file that can be visualized. This fix also has some significant code cleanup. --- tools/js-dep-visualizer.py | 353 +++++++++++++++++++++++++------------ tools/lib/graph.py | 59 ++++++- 2 files changed, 292 insertions(+), 120 deletions(-) diff --git a/tools/js-dep-visualizer.py b/tools/js-dep-visualizer.py index 27f02d4e98..2f37aca0cb 100644 --- a/tools/js-dep-visualizer.py +++ b/tools/js-dep-visualizer.py @@ -9,143 +9,266 @@ from __future__ import print_function import os import re import sys +from collections import defaultdict + +from typing import Any, DefaultDict, Dict, List, Set, Tuple +Edge = Tuple[str, str] +EdgeSet = Set[Edge] +Method = str +MethodDict = DefaultDict[Edge, List[Method]] -from typing import Any, Dict, List TOOLS_DIR = os.path.abspath(os.path.dirname(__file__)) ROOT_DIR = os.path.dirname(TOOLS_DIR) sys.path.insert(0, ROOT_DIR) -from tools.lib.graph import Graph, make_dot_file +from tools.lib.graph import ( + Graph, + make_dot_file, + best_edge_to_remove, +) JS_FILES_DIR = os.path.join(ROOT_DIR, 'static/js') OUTPUT_FILE_PATH = os.path.relpath(os.path.join(ROOT_DIR, 'var/zulip-deps.dot')) -names = set() -modules = [] # type: List[Dict[str, Any]] -for js_file in os.listdir(JS_FILES_DIR): - if not js_file.endswith('.js'): - continue - name = js_file[:-3] # remove .js - path = os.path.join(JS_FILES_DIR, js_file) - names.add(name) - modules.append(dict( - name=name, - path=path, - regex=re.compile('[^_]{}\.\w+\('.format(name)) - )) +def get_js_edges(): + # type: () -> Tuple[EdgeSet, MethodDict] + names = set() + modules = [] # type: List[Dict[str, Any]] + for js_file in os.listdir(JS_FILES_DIR): + if not js_file.endswith('.js'): + continue + name = js_file[:-3] # remove .js + path = os.path.join(JS_FILES_DIR, js_file) + names.add(name) + modules.append(dict( + name=name, + path=path, + regex=re.compile('[^_]{}\.\w+\('.format(name)) + )) -COMMENT_REGEX = re.compile('\s+//') -REGEX = re.compile('[^_](\w+)\.\w+\(') + comment_regex = re.compile('\s+//') + call_regex = re.compile('[^_](\w+\.\w+)\(') -tuples = set() -for module in modules: - parent = module['name'] + methods = defaultdict(list) # type: DefaultDict[Edge, List[Method]] + edges = set() + for module in modules: + parent = module['name'] - with open(module['path']) as f: - for line in f: - if COMMENT_REGEX.match(line): - continue - if 'subs.forEach' in line: - continue - m = REGEX.search(line) - if not m: - continue - for child in m.groups(): - if (child in names) and (child != parent): + with open(module['path']) as f: + for line in f: + if comment_regex.match(line): + continue + if 'subs.forEach' in line: + continue + m = call_regex.search(line) + if not m: + continue + for g in m.groups(): + child, method = g.split('.') + if (child not in names): + continue + if child == parent: + continue tup = (parent, child) - tuples.add(tup) + edges.add(tup) + methods[tup].append(method) + return edges, methods -IGNORE_TUPLES = [ - # We ignore the following tuples to de-clutter the graph, since there is a - # pretty clear roadmap on how to break the dependencies. You can comment - # these out to see what the "real" situation looks like now, and if you do - # the work of breaking the dependency, you can remove it. +def find_edges_to_remove(graph, methods): + # type: (Graph, MethodDict) -> Tuple[Graph, List[Edge]] + EXEMPT_EDGES = [ + # These are sensible dependencies, so don't cut them. + ('rows', 'message_store'), + ('filter', 'stream_data'), + ('server_events', 'user_events'), + ('compose_fade', 'stream_data'), + ('narrow', 'message_list'), + ('stream_list', 'topic_list',), + ('subs', 'stream_muting'), + ('hashchange', 'settings'), + ('tutorial', 'narrow'), + ('activity', 'resize'), + ('hashchange', 'drafts'), + ('compose', 'echo'), + ('compose', 'resize'), + ('compose', 'unread_ops'), + ('compose', 'drafts'), + ('echo', 'message_edit'), + ('echo', 'stream_list'), + ('hashchange', 'narrow'), + ('hashchange', 'subs'), + ('message_edit', 'echo'), + ('popovers', 'message_edit'), + ('unread_ui', 'activity'), + ('message_fetch', 'message_util'), + ('message_fetch', 'resize'), + ('message_util', 'resize'), + ('notifications', 'tutorial'), + ('message_util', 'unread_ui'), + ('muting_ui', 'stream_list'), + ('muting_ui', 'unread_ui'), + ('stream_popover', 'subs'), + ('stream_popover', 'muting_ui'), + ('narrow', 'message_fetch'), + ('narrow', 'message_util'), + ('narrow', 'navigate'), + ('unread_ops', 'unread_ui'), + ('narrow', 'unread_ops'), + ('navigate', 'unread_ops'), + ('pm_list', 'unread_ui'), + ('stream_list', 'unread_ui'), + ('popovers', 'compose'), + ('popovers', 'muting_ui'), + ('popovers', 'narrow'), + ('popovers', 'resize'), + ('pm_list', 'resize'), + ('notifications', 'navigate'), + ('compose', 'socket'), + ('referral', 'resize'), + ('stream_muting', 'message_util'), + ('subs', 'stream_list'), + ('ui', 'message_fetch'), + ('ui', 'unread_ops'), + ('condense', 'message_viewport'), + ] # type: List[Edge] - ('echo', 'message_events'), # do something slimy here - ('echo', 'ui'), + def is_exempt(edge): + # type: (Tuple[str, str]) -> bool + parent, child = edge + if edge == ('server_events', 'reload'): + return False + if parent in ['server_events', 'user_events', 'stream_events', + 'message_events', 'reload']: + return True + if child == 'rows': + return True + return edge in EXEMPT_EDGES - ('stream_data', 'narrow'), # split out narrow.by_foo functions - ('activity', 'narrow'), + APPROVED_CUTS = [ + ('echo', 'message_events'), + ('resize', 'navigate'), + ('narrow', 'search'), + ('subs', 'stream_events'), + ('stream_color', 'tab_bar'), + ('stream_color', 'subs'), + ('stream_data', 'narrow'), + ('unread', 'narrow'), + ('composebox_typeahead', 'compose'), + ('message_list', 'message_edit'), + ('message_edit', 'compose'), + ('message_store', 'compose'), + ('settings', 'muting_ui'), + ('message_fetch', 'tutorial'), + ('settings', 'subs'), + ('activity', 'narrow'), + ('compose', 'subs'), + ('drafts', 'compose'), + ('drafts', 'echo'), + ('echo', 'compose'), + ('echo', 'narrow'), + ('echo', 'pm_list'), + ('echo', 'ui'), + ('message_fetch', 'activity'), + ('message_fetch', 'narrow'), + ('message_fetch', 'pm_list'), + ('message_fetch', 'stream_list'), + ('message_fetch', 'ui'), + ('narrow', 'ui'), + ('message_util', 'compose'), + ('subs', 'compose'), + ('narrow', 'hashchange'), + ('subs', 'hashchange'), + ('navigate', 'narrow'), + ('navigate', 'stream_list'), + ('pm_list', 'narrow'), + ('pm_list', 'stream_popover'), + ('popovers', 'stream_popover'), + ('topic_list', 'stream_popover'), + ('topic_list', 'narrow'), + ('stream_list', 'narrow'), + ('stream_list', 'pm_list'), + ('stream_list', 'unread_ops'), + ('notifications', 'ui'), + ('notifications', 'narrow'), + ('notifications', 'unread_ops'), + ('typing', 'narrow'), + ('message_events', 'compose'), + ('stream_muting', 'stream_list'), + ('subs', 'narrow'), + ('unread_ui', 'pm_list'), + ('unread_ui', 'stream_list'), + ] - ('subs', 'narrow'), # data functions - ('subs', 'compose'), # data functions + def cut_is_legal(edge): + # type: (Edge) -> bool + parent, child = edge + if child in ['reload', 'popovers', 'modals', 'notifications', 'server_events']: + return True + return edge in APPROVED_CUTS - ('narrow', 'ui'), # just three functions + graph.remove_exterior_nodes() + removed_edges = list() + print() + while graph.num_edges() > 0: + edge = best_edge_to_remove(graph, is_exempt) + if edge is None: + print('we may not be allowing edge cuts!!!') + break + if cut_is_legal(edge): + graph = graph.minus_edge(edge) + graph.remove_exterior_nodes() + removed_edges.append(edge) + else: + for removed_edge in removed_edges: + print(removed_edge) + print() + edge_str = str(edge) + ',' + print(edge_str) + for method in methods[edge]: + print(' ' + method) + break - ('stream_data', 'stream_color'), # split out stream_color data/UI - ('stream_color', 'tab_bar'), # only one call - ('stream_color', 'subs'), # only one call + return graph, removed_edges - ('subs', 'stream_events'), # see TODOs related to mark_{un,}subscribed +def report_roadmap(edges, methods): + # type: (List[Edge], MethodDict) -> None + child_modules = {child for parent, child in edges} + module_methods = defaultdict(set) # type: DefaultDict[str, Set[str]] + callers = defaultdict(set) # type: DefaultDict[Tuple[str, str], Set[str]] + for parent, child in edges: + for method in methods[(parent, child)]: + module_methods[child].add(method) + callers[(child, method)].add(parent) - ('subs', 'hashchange'), # modal stuff + for child in sorted(child_modules): + print(child + '.js') + for method in module_methods[child]: + print(' ' + child + '.' + method) + for caller in sorted(callers[(child, method)]): + print(' ' + caller + '.js') + print() + print() - ('message_store', 'compose'), # split out compose_data +def produce_partial_output(graph): + # type: (Graph) -> None + print(graph.num_edges()) + buffer = make_dot_file(graph) - ('search', 'search_suggestion'), # move handler into search_suggestion + graph.report() + with open(OUTPUT_FILE_PATH, 'w') as f: + f.write(buffer) + print() + print('see dot file here: {}'.format(OUTPUT_FILE_PATH)) - ('unread', 'narrow'), # create narrow_state.js +def run(): + # type: () -> None + edges, methods = get_js_edges() + graph = Graph(edges) + graph, removed_edges = find_edges_to_remove(graph, methods) + if graph.num_edges() == 0: + report_roadmap(removed_edges, methods) + else: + produce_partial_output(graph) - ('navigate', 'stream_list'), # move cycle_stream into stream_list.js - - # This one is kind of tricky, but basically we want to split all the basic - # code out of both of these modules that's essentially just string manipulation. - ('narrow', 'hashchange'), - - ('composebox_typeahead', 'compose'), # introduce compose_state.js - - # This one might require some work, but the idea is to split out something - # like narrow_state.js. - ('pm_list', 'narrow'), - - ('settings', 'subs'), # not much to fix, can call stream_data directly, maybe - - ('modals', 'subs'), # add some kind of onClose mechanism in new modals.open - - ('channel', 'reload'), # just one call to fix somehow - ('compose', 'reload'), # use channel stuff more directly? - - ('settings', 'muting_ui'), # inline call or split out muting_settings.js - - ('resize', 'navigate'), # split out scroll.js - ('resize', 'popovers'), # only three interactions - -] - -for tup in IGNORE_TUPLES: - try: - tuples.remove(tup) - except KeyError: - print(''' - {} no longer needs to be ignored. Help us celebrate - by removing it from IGNORE_TUPLES! - '''.format(tup)) - sys.exit(1) - - -# print(tuples) -graph = Graph(*tuples) -ignore_modules = [ - 'blueslip', - 'message_edit', - 'message_util', - 'modals', - 'notifications', - 'popovers', - 'server_events', - 'stream_popover', - 'topic_list', - 'tutorial', - 'unread_ops', - 'rows', # message_store -] -for node in ignore_modules: - graph.remove(node) -graph.remove_exterior_nodes() -graph.report() -buffer = make_dot_file(graph) - -with open(OUTPUT_FILE_PATH, 'w') as f: - f.write(buffer) -print() -print('see dot file here: {}'.format(OUTPUT_FILE_PATH)) +if __name__ == '__main__': + run() diff --git a/tools/lib/graph.py b/tools/lib/graph.py index 1d1adb546d..59c02f27dc 100644 --- a/tools/lib/graph.py +++ b/tools/lib/graph.py @@ -3,11 +3,14 @@ from __future__ import print_function from collections import defaultdict -from typing import DefaultDict, List, Set, Tuple +from typing import Callable, DefaultDict, Iterator, List, Set, Tuple + +Edge = Tuple[str, str] +EdgeSet = Set[Edge] class Graph(object): - def __init__(self, *tuples): - # type: (Tuple[str, str]) -> None + def __init__(self, tuples): + # type: (EdgeSet) -> None self.children = defaultdict(list) # type: DefaultDict[str, List[str]] self.parents = defaultdict(list) # type: DefaultDict[str, List[str]] self.nodes = set() # type: Set[str] @@ -18,6 +21,28 @@ class Graph(object): self.nodes.add(parent) self.nodes.add(child) + def copy(self): + # type: () -> Graph + return Graph(self.edges()) + + def num_edges(self): + # type: () -> int + return len(self.edges()) + + def minus_edge(self, edge): + # type: (Edge) -> Graph + edges = self.edges().copy() + edges.remove(edge) + return Graph(edges) + + def edges(self): + # type: () -> EdgeSet + s = set() + for parent in self.nodes: + for child in self.children[parent]: + s.add((parent, child)) + return s + def remove_exterior_nodes(self): # type: () -> None still_work_to_do = True @@ -61,6 +86,30 @@ class Graph(object): for tup in tups: print(tup) +def best_edge_to_remove(orig_graph, is_exempt): + # type: (Graph, Callable[[Edge], bool]) -> Edge + # expects an already reduced graph as input + + orig_edges = orig_graph.edges() + + def get_choices(): + # type: () -> Iterator[Tuple[int, Edge]] + for edge in orig_edges: + if is_exempt(edge): + continue + graph = orig_graph.minus_edge(edge) + graph.remove_exterior_nodes() + size = graph.num_edges() + yield (size, edge) + + choices = list(get_choices()) + if not choices: + return None + min_size, best_edge = min(choices) + if min_size >= orig_graph.num_edges(): + raise Exception('no edges work here') + return best_edge + def make_dot_file(graph): # type: (Graph) -> str buffer = 'digraph G {\n' @@ -73,7 +122,7 @@ def make_dot_file(graph): def test(): # type: () -> None - graph = Graph( + graph = Graph(set([ ('x', 'a'), ('a', 'b'), ('b', 'c'), @@ -82,7 +131,7 @@ def test(): ('d', 'e'), ('e', 'f'), ('e', 'g'), - ) + ])) graph.remove_exterior_nodes() s = make_dot_file(graph)