From b97121ccdf6ffb15dc595cbaf6e0d4533c3391fa Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 31 Jul 2023 21:22:42 -0700 Subject: [PATCH 1/4] Cache known ancestors and don't wastefully traverse --- plugin/src/PatchTree.lua | 48 +++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index b241472d0..0c5edcb38 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -122,16 +122,7 @@ function Tree:addNode(parent, props) parent = parent or "ROOT" - local node = self:getNode(props.id) - if node then - -- Update existing node - for k, v in props do - node[k] = v - end - return node - end - - node = table.clone(props) + local node = table.clone(props) node.children = {} node.parentId = parent @@ -149,9 +140,10 @@ end -- Given a list of ancestor ids in descending order, builds the nodes for them -- using the patch and instanceMap info -function Tree:buildAncestryNodes(ancestryIds: { string }, patch, instanceMap) +function Tree:buildAncestryNodes(previousId: string?, ancestryIds: { string }, patch, instanceMap) -- Build nodes for ancestry by going up the tree - local previousId = "ROOT" + previousId = previousId or "ROOT" + for _, ancestorId in ancestryIds do local value = instanceMap.fromIds[ancestorId] or patch.added[ancestorId] if not value then @@ -175,6 +167,8 @@ local PatchTree = {} function PatchTree.build(patch, instanceMap, changeListHeaders) local tree = Tree.new() + local knownAncestors = {} + for _, change in patch.updated do local instance = instanceMap.fromIds[change.id] if not instance then @@ -185,13 +179,21 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) local ancestryIds = {} local parentObject = instance.Parent local parentId = instanceMap.fromInstances[parentObject] + local previousId = nil while parentObject do + if knownAncestors[parentId] then + -- We've already added this ancestor + previousId = parentId + break + end + table.insert(ancestryIds, 1, parentId) + knownAncestors[parentId] = true parentObject = parentObject.Parent parentId = instanceMap.fromInstances[parentObject] end - tree:buildAncestryNodes(ancestryIds, patch, instanceMap) + tree:buildAncestryNodes(previousId, ancestryIds, patch, instanceMap) -- Gather detail text local changeList, hint = nil, nil @@ -268,14 +270,22 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) local ancestryIds = {} local parentObject = instance.Parent local parentId = instanceMap.fromInstances[parentObject] or HttpService:GenerateGUID(false) + local previousId = nil while parentObject do + if knownAncestors[parentId] then + -- We've already added this ancestor + previousId = parentId + break + end + instanceMap:insert(parentId, parentObject) -- This ensures we can find the parent later table.insert(ancestryIds, 1, parentId) + knownAncestors[parentId] = true parentObject = parentObject.Parent parentId = instanceMap.fromInstances[parentObject] or HttpService:GenerateGUID(false) end - tree:buildAncestryNodes(ancestryIds, patch, instanceMap) + tree:buildAncestryNodes(previousId, ancestryIds, patch, instanceMap) -- Add this node to tree local nodeId = instanceMap.fromInstances[instance] or HttpService:GenerateGUID(false) @@ -295,8 +305,16 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) local parentId = change.Parent local parentData = patch.added[parentId] local parentObject = instanceMap.fromIds[parentId] + local previousId = nil while parentId do + if knownAncestors[parentId] then + -- We've already added this ancestor + previousId = parentId + break + end + table.insert(ancestryIds, 1, parentId) + knownAncestors[parentId] = true parentId = nil if parentData then @@ -312,7 +330,7 @@ function PatchTree.build(patch, instanceMap, changeListHeaders) end end - tree:buildAncestryNodes(ancestryIds, patch, instanceMap) + tree:buildAncestryNodes(previousId, ancestryIds, patch, instanceMap) -- Gather detail text local changeList, hint = nil, nil From bcc0024a014045a1fbd8a9acf8cb10800850ff40 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 31 Jul 2023 21:43:17 -0700 Subject: [PATCH 2/4] Support updating nodes but without full searches on misses --- plugin/src/PatchTree.lua | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index 0c5edcb38..0ca09a316 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -113,6 +113,10 @@ function Tree:getNode(id, searchNode) return nil end +function Tree:doesNodeExist(id) + return self.idToNode[id] ~= nil +end + -- Adds a node to the tree as a child of the node with id == parent -- If parent is nil, it defaults to root -- props must contain id, and cannot contain children or parentId @@ -122,6 +126,15 @@ function Tree:addNode(parent, props) parent = parent or "ROOT" + if self:doesNodeExist(props.id) then + -- Update existing node + local node = self:getNode(props.id) + for k, v in props do + node[k] = v + end + return node + end + local node = table.clone(props) node.children = {} node.parentId = parent From 53a976b996a997c2e0a6f171224f6219d61f7d9b Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 31 Jul 2023 21:46:21 -0700 Subject: [PATCH 3/4] Fix whitespace --- plugin/src/PatchTree.lua | 104 +++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index 0ca09a316..568f2412a 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -64,18 +64,18 @@ local Tree = {} Tree.__index = Tree function Tree.new() - local tree = { - idToNode = {}, + local tree = { + idToNode = {}, ROOT = { className = "DataModel", name = "ROOT", children = {}, }, - } - -- Add ROOT to idToNode or it won't be found by getNode since that searches *within* ROOT + } + -- Add ROOT to idToNode or it won't be found by getNode since that searches *within* ROOT tree.idToNode["ROOT"] = tree.ROOT - return setmetatable(tree, Tree) + return setmetatable(tree, Tree) end -- Iterates over all sub-nodes, depth first @@ -94,23 +94,23 @@ end -- Finds a node by id, depth first -- searchNode is the node to start the search within, defaults to root function Tree:getNode(id, searchNode) - if self.idToNode[id] then - return self.idToNode[id] - end + if self.idToNode[id] then + return self.idToNode[id] + end local searchChildren = (searchNode or self.ROOT).children - for nodeId, node in searchChildren do - if nodeId == id then - self.idToNode[id] = node - return node - end - local descendant = self:getNode(id, node) - if descendant then - return descendant - end - end - - return nil + for nodeId, node in searchChildren do + if nodeId == id then + self.idToNode[id] = node + return node + end + local descendant = self:getNode(id, node) + if descendant then + return descendant + end + end + + return nil end function Tree:doesNodeExist(id) @@ -124,53 +124,53 @@ end function Tree:addNode(parent, props) assert(props.id, "props must contain id") - parent = parent or "ROOT" + parent = parent or "ROOT" if self:doesNodeExist(props.id) then -- Update existing node local node = self:getNode(props.id) - for k, v in props do - node[k] = v - end - return node + for k, v in props do + node[k] = v + end + return node end - local node = table.clone(props) - node.children = {} + local node = table.clone(props) + node.children = {} node.parentId = parent - local parentNode = self:getNode(parent) - if not parentNode then - Log.warn("Failed to create node since parent doesnt exist: {}, {}", parent, props) - return - end + local parentNode = self:getNode(parent) + if not parentNode then + Log.warn("Failed to create node since parent doesnt exist: {}, {}", parent, props) + return + end - parentNode.children[node.id] = node - self.idToNode[node.id] = node + parentNode.children[node.id] = node + self.idToNode[node.id] = node - return node + return node end -- Given a list of ancestor ids in descending order, builds the nodes for them -- using the patch and instanceMap info function Tree:buildAncestryNodes(previousId: string?, ancestryIds: { string }, patch, instanceMap) - -- Build nodes for ancestry by going up the tree - previousId = previousId or "ROOT" - - for _, ancestorId in ancestryIds do - local value = instanceMap.fromIds[ancestorId] or patch.added[ancestorId] - if not value then - Log.warn("Failed to find ancestor object for " .. ancestorId) - continue - end - self:addNode(previousId, { - id = ancestorId, - className = value.ClassName, - name = value.Name, - instance = if typeof(value) == "Instance" then value else nil, - }) - previousId = ancestorId - end + -- Build nodes for ancestry by going up the tree + previousId = previousId or "ROOT" + + for _, ancestorId in ancestryIds do + local value = instanceMap.fromIds[ancestorId] or patch.added[ancestorId] + if not value then + Log.warn("Failed to find ancestor object for " .. ancestorId) + continue + end + self:addNode(previousId, { + id = ancestorId, + className = value.ClassName, + name = value.Name, + instance = if typeof(value) == "Instance" then value else nil, + }) + previousId = ancestorId + end end local PatchTree = {} From 2205c4bad7e60be677ebca699e06c04b4f53f539 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 31 Jul 2023 21:54:43 -0700 Subject: [PATCH 4/4] Add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88ea4e416..a4e2803a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ * Added better support for `Font` properties ([#731]) * Add new plugin template to the `init` command ([#738]) * Added rich Source diffs in patch visualizer ([#748]) +* Fix PatchTree performance issues ([#755]) [#745]: https://github.com/rojo-rbx/rojo/pull/745 [#668]: https://github.com/rojo-rbx/rojo/pull/668 @@ -44,6 +45,7 @@ [#731]: https://github.com/rojo-rbx/rojo/pull/731 [#738]: https://github.com/rojo-rbx/rojo/pull/738 [#748]: https://github.com/rojo-rbx/rojo/pull/748 +[#755]: https://github.com/rojo-rbx/rojo/pull/755 ## [7.3.0] - April 22, 2023 * Added `$attributes` to project format. ([#574])