goalc: Fix new symbol trie's performance inefficiencies (#3443)

I believe this brings things back in line to where it was before:
Here are the first handful of files before the changes:
```
0.014 | gcommon.gc
0.006 | gkernel-h.gc
0.025 | gkernel.gc
0.002 | pskernel.gc
0.01 | gstring.gc
0.004 | gstate.gc
0.001 | kernel.gd
0.001 | types-h.gc
0.002 | vu1-macros.gc
0.003 | math.gc
0.01 | vector-h.gc
0.001 | gravity-h.gc
0.001 | bounding-box-h.gc
0.001 | matrix-h.gc
0.001 | quaternion-h.gc
0.001 | euler-h.gc
```
> first compile
```
0.161 | gcommon.gc
0.126 | gkernel-h.gc
0.174 | gkernel.gc
0.046 | pskernel.gc
0.08 | gstring.gc
0.048 | gstate.gc
0.001 | kernel.gd
0.052 | types-h.gc
0.009 | vu1-macros.gc
0.059 | math.gc
0.228 | vector-h.gc
0.026 | gravity-h.gc
0.006 | bounding-box-h.gc
0.002 | matrix-h.gc
0.028 | quaternion-h.gc
0.026 | euler-h.gc
```
> make a change in gcommon and recompile

With the changes:
```
0.015 | gcommon.gc
0.018 | gkernel-h.gc
0.039 | gkernel.gc
0.006 | pskernel.gc
0.015 | gstring.gc
0.009 | gstate.gc
0.005 | kernel.gd
0.006 | types-h.gc
0.006 | vu1-macros.gc
0.008 | math.gc
0.017 | vector-h.gc
0.004 | gravity-h.gc
0.004 | bounding-box-h.gc
0.005 | matrix-h.gc
0.005 | quaternion-h.gc
0.003 | euler-h.gc
```
> First compile, no difference expected

```
0.016 | gcommon.gc
0.008 | gkernel-h.gc
0.023 | gkernel.gc
0.002 | pskernel.gc
0.01 | gstring.gc
0.043 | gstate.gc
0.001 | kernel.gd
0.002 | types-h.gc
0.002 | vu1-macros.gc
0.003 | math.gc
0.013 | vector-h.gc
0.001 | gravity-h.gc
0.002 | bounding-box-h.gc
0.002 | matrix-h.gc
0.001 | quaternion-h.gc
0.001 | euler-h.gc
```
> Compile times seem to be back within margin of error -- some are
faster than the first compilation time.
This commit is contained in:
Tyler Wilding
2024-04-01 18:56:55 -04:00
committed by GitHub
parent c96e44ab30
commit 0a15ac1669
10 changed files with 172 additions and 210 deletions
+1 -2
View File
@@ -87,8 +87,7 @@ add_library(common
util/term_util.cpp
util/Timer.cpp
util/unicode_util.cpp
versions/versions.cpp
"util/trie_map.h")
versions/versions.cpp)
target_link_libraries(common fmt lzokay replxx libzstd_static tree-sitter sqlite3 libtinyfiledialogs)
-160
View File
@@ -1,160 +0,0 @@
#pragma once
#include <algorithm>
#include <iostream>
#include <memory>
#include <string>
#include <unordered_map>
#include <vector>
// TrieMap class
template <typename T>
class TrieMap {
private:
// TrieNode structure
struct TrieNode {
std::unordered_map<char, std::shared_ptr<TrieNode>> children;
std::vector<std::shared_ptr<T>> elements;
};
std::shared_ptr<TrieNode> root;
public:
TrieMap() : root(std::make_shared<TrieNode>()) {}
// Insert an element with a key into the TrieMap and return the inserted element
std::shared_ptr<T> insert(const std::string& key, const T& element) {
std::shared_ptr<T> shared_element = std::make_shared<T>(element);
std::shared_ptr<TrieNode> node = root;
for (char c : key) {
if (node->children.find(c) == node->children.end()) {
node->children[c] = std::make_shared<TrieNode>();
}
node = node->children[c];
}
// Store element at the leaf node
node->elements.push_back(shared_element);
return shared_element;
}
// Retrieve elements with a given prefix
std::vector<std::shared_ptr<T>> retrieve_with_prefix(const std::string& prefix) const {
std::vector<std::shared_ptr<T>> result;
std::shared_ptr<TrieNode> node = root;
// Traverse to the node representing the prefix
for (char c : prefix) {
if (node->children.find(c) == node->children.end()) {
return result; // No elements with the given prefix
}
node = node->children[c];
}
// Gather all elements stored at or below this node
retrieve_elements(node, result);
return result;
}
// Retrieve elements with an exact key match
std::vector<std::shared_ptr<T>> retrieve_with_exact(const std::string& key) const {
std::vector<std::shared_ptr<T>> result;
std::shared_ptr<TrieNode> node = root;
// Traverse to the node representing the key
for (char c : key) {
if (node->children.find(c) == node->children.end()) {
return result; // No elements with the given key
}
node = node->children[c];
}
// Return elements stored at this node
return node->elements;
}
// Remove the specified element from the TrieMap
void remove(const std::shared_ptr<T>& element) { remove_element(root, element); }
// Return the total number of elements stored in the TrieMap
int size() const {
int count = 0;
count_elements(root, count);
return count;
}
// Return a vector containing shared pointers to all elements stored in the TrieMap
std::vector<std::shared_ptr<T>> get_all_elements() const {
std::vector<std::shared_ptr<T>> result;
get_all_elements_helper(root, result);
return result;
}
private:
// Recursive function to retrieve elements stored at or below the given node
void retrieve_elements(std::shared_ptr<TrieNode> node,
std::vector<std::shared_ptr<T>>& result) const {
// Add elements stored at this node to the result
for (const auto& element : node->elements) {
result.push_back(element);
}
// Recursively traverse children
for (const auto& child : node->children) {
retrieve_elements(child.second, result);
}
}
// Recursive function to remove the specified element from the TrieMap
bool remove_element(std::shared_ptr<TrieNode> node, const std::shared_ptr<T>& element) {
// Remove the element if it exists at this node
auto& elements = node->elements;
auto it = std::find(elements.begin(), elements.end(), element);
if (it != elements.end()) {
elements.erase(it);
return true;
}
// Recursively search children
for (auto& child : node->children) {
if (remove_element(child.second, element)) {
// Remove child node if it's empty after removal
if (child.second->elements.empty() && child.second->children.empty()) {
node->children.erase(child.first);
}
return true;
}
}
return false;
}
// Recursive function to count elements stored at or below the given node
void count_elements(std::shared_ptr<TrieNode> node, int& count) const {
// Increment count by the number of elements stored at this node
count += node->elements.size();
// Recursively traverse children
for (const auto& child : node->children) {
count_elements(child.second, count);
}
}
// Recursive helper function to collect all elements stored in the TrieMap
void get_all_elements_helper(std::shared_ptr<TrieNode> node,
std::vector<std::shared_ptr<T>>& result) const {
// Add elements stored at this node to the result
for (const auto& element : node->elements) {
result.push_back(element);
}
// Recursively traverse children
for (const auto& child : node->children) {
get_all_elements_helper(child.second, result);
}
}
};
// TrieMap<std::string> trie_map;
//
//// Insert elements
// std::shared_ptr<std::string> inserted_element_1 = trie_map.insert("apple", "A fruit");
// std::shared_ptr<std::string> inserted_element_2 = trie_map.insert("app", "An application");
// std::shared_ptr<std::string> inserted_element_3 = trie_map.insert("banana", "Another fruit");
// std::shared_ptr<std::string> inserted_element_4 = trie_map.insert("apple", "Another apple");
//
//// Remove an element
// trie_map.remove(inserted_element_1);
//
//// Retrieve elements with a prefix
// std::vector<std::shared_ptr<std::string>> prefix_results = trie_map.retrieve_with_prefix("app");
+134
View File
@@ -0,0 +1,134 @@
#pragma once
#include <array>
#include <memory>
#include <string>
#include <vector>
// A normal Trie does not allow for duplicate keys, however this one does
// It allows for insertion and removal
//
// Note, keys assume only basic ASCII, which is _fine_ as OpenGOAL itself has this
// limitation as well.
template <typename T>
class TrieWithDuplicates {
private:
struct TrieNode {
std::array<std::unique_ptr<TrieNode>, 256> children;
std::vector<std::unique_ptr<T>> elements;
};
std::unique_ptr<TrieNode> root = std::make_unique<TrieNode>();
public:
TrieWithDuplicates() {}
T* insert(const std::string& key, const T& element) {
std::unique_ptr<T> new_element = std::make_unique<T>(element);
TrieNode* curr_node = root.get();
for (const char character : key) {
auto& child = curr_node->children[(uint8_t)character];
if (!child) {
child = std::make_unique<TrieNode>();
}
curr_node = child.get();
}
curr_node->elements.push_back(std::move(new_element));
return curr_node->elements.back().get();
}
std::vector<T*> retrieve_with_prefix(const std::string& prefix) const {
std::vector<T*> results;
TrieNode* curr_node = root.get();
for (const char character : prefix) {
const auto& child = curr_node->children.at((uint8_t)character);
if (child == nullptr) {
return results; // tree ends, nothing found with that prefix
} else {
curr_node = child.get();
}
}
retrieve_elements(curr_node, results);
return results;
}
std::vector<T*> retrieve_with_exact(const std::string& key) const {
std::vector<T*> results;
TrieNode* curr_node = root.get();
for (const char character : key) {
const auto& child = curr_node->children.at((uint8_t)character);
if (child == nullptr) {
return results; // tree ends, nothing found with that key
} else {
curr_node = child.get();
}
}
for (const auto& element : curr_node->elements) {
results.push_back(element.get());
}
return results;
}
bool remove(const std::string& key, const T* to_be_removed) {
TrieNode* curr_node = root.get();
for (const char character : key) {
const auto& child = curr_node->children.at((uint8_t)character);
if (child == nullptr) {
return false; // tree ends, nothing found with that key
} else {
curr_node = child.get();
}
}
// Since the trie holds duplicates, we can't delete on the key alone
// now search to see which element is identical
auto it = curr_node->elements.begin();
while (it != curr_node->elements.end()) {
if (it->get() == to_be_removed) {
it = curr_node->elements.erase(it);
return true; // we can assume that the same ptr isn't stored twice.
} else {
++it;
}
}
return false;
}
// Return the total number of elements stored in the TrieMap
int size() const {
int count = 0;
count_elements(root.get(), count);
return count;
}
std::vector<T*> get_all_elements() const {
std::vector<T*> results;
get_all_elements_helper(root.get(), results);
return results;
}
private:
void retrieve_elements(const TrieNode* node, std::vector<T*>& results) const {
for (const auto& element : node->elements) {
results.push_back(element.get());
}
for (const auto& child : node->children) {
retrieve_elements(child.get(), results);
}
}
void count_elements(const TrieNode* node, int& count) const {
count += node->elements.size();
for (const auto& child : node->children) {
count_elements(child.get(), count);
}
}
void get_all_elements_helper(const TrieNode* node, std::vector<T*>& result) const {
for (const auto& element : node->elements) {
result.push_back(element.get());
}
for (const auto& child : node->children) {
get_all_elements_helper(child.get(), result);
}
}
};
+4 -5
View File
@@ -97,13 +97,12 @@ class Compiler {
std::vector<std::pair<std::string, replxx::Replxx::Color>> const& user_data);
bool knows_object_file(const std::string& name);
MakeSystem& make_system() { return m_make; }
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> lookup_symbol_info_by_file(
std::vector<symbol_info::SymbolInfo*> lookup_symbol_info_by_file(
const std::string& file_path) const;
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> lookup_symbol_info_by_prefix(
std::vector<symbol_info::SymbolInfo*> lookup_symbol_info_by_prefix(
const std::string& prefix) const;
std::set<std::string> lookup_symbol_names_starting_with(const std::string& prefix) const;
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> lookup_exact_name_info(
const std::string& name) const;
std::vector<symbol_info::SymbolInfo*> lookup_exact_name_info(const std::string& name) const;
std::optional<TypeSpec> lookup_typespec(const std::string& symbol_name);
TypeSystem& type_system() { return m_ts; };
// TODO - rename these types / namespaces -- consolidate with SymbolInfo and whatever else tries
@@ -321,7 +320,7 @@ class Compiler {
int offset,
Env* env);
std::string make_symbol_info_description(const std::shared_ptr<symbol_info::SymbolInfo> info);
std::string make_symbol_info_description(const symbol_info::SymbolInfo* info);
MathMode get_math_mode(const TypeSpec& ts);
bool is_number(const TypeSpec& ts);
@@ -358,8 +358,7 @@ Val* Compiler::compile_reload(const goos::Object& form, const goos::Object& rest
return get_none();
}
std::string Compiler::make_symbol_info_description(
const std::shared_ptr<symbol_info::SymbolInfo> info) {
std::string Compiler::make_symbol_info_description(const symbol_info::SymbolInfo* info) {
switch (info->m_kind) {
case symbol_info::Kind::GLOBAL_VAR:
return fmt::format("[Global Variable] Type: {} Defined: {}",
@@ -588,12 +587,12 @@ Val* Compiler::compile_update_macro_metadata(const goos::Object& form,
return get_none();
}
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> Compiler::lookup_symbol_info_by_file(
std::vector<symbol_info::SymbolInfo*> Compiler::lookup_symbol_info_by_file(
const std::string& file_path) const {
return m_symbol_info.lookup_symbols_by_file(file_path);
}
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> Compiler::lookup_symbol_info_by_prefix(
std::vector<symbol_info::SymbolInfo*> Compiler::lookup_symbol_info_by_prefix(
const std::string& prefix) const {
return m_symbol_info.lookup_symbols_starting_with(prefix);
}
@@ -605,7 +604,7 @@ std::set<std::string> Compiler::lookup_symbol_names_starting_with(const std::str
return {};
}
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> Compiler::lookup_exact_name_info(
std::vector<symbol_info::SymbolInfo*> Compiler::lookup_exact_name_info(
const std::string& name) const {
if (m_goos.reader.check_string_is_valid(name)) {
return m_symbol_info.lookup_exact_name(name);
+10 -13
View File
@@ -57,8 +57,7 @@ void SymbolInfo::set_definition_location(const goos::TextDb* textdb) {
}
}
void SymbolInfoMap::add_symbol_to_file_index(const std::string& file_path,
std::shared_ptr<SymbolInfo> symbol) {
void SymbolInfoMap::add_symbol_to_file_index(const std::string& file_path, SymbolInfo* symbol) {
if (m_file_symbol_index.find(file_path) == m_file_symbol_index.end()) {
m_file_symbol_index[file_path] = {};
}
@@ -266,22 +265,20 @@ void SymbolInfoMap::add_method(const std::string& method_name,
}
}
std::vector<std::shared_ptr<SymbolInfo>> SymbolInfoMap::lookup_symbols_by_file(
const std::string& file_path) const {
std::vector<SymbolInfo*> SymbolInfoMap::lookup_symbols_by_file(const std::string& file_path) const {
if (m_file_symbol_index.find(file_path) != m_file_symbol_index.end()) {
return m_file_symbol_index.at(file_path);
}
return {};
}
std::vector<std::shared_ptr<SymbolInfo>> SymbolInfoMap::lookup_exact_name(
const std::string& name) const {
std::vector<SymbolInfo*> SymbolInfoMap::lookup_exact_name(const std::string& name) const {
return m_symbol_map.retrieve_with_exact(name);
}
std::vector<std::shared_ptr<SymbolInfo>> SymbolInfoMap::lookup_symbols_starting_with(
std::vector<SymbolInfo*> SymbolInfoMap::lookup_symbols_starting_with(
const std::string& prefix) const {
std::vector<std::shared_ptr<SymbolInfo>> symbols;
std::vector<SymbolInfo*> symbols;
const auto lookup = m_symbol_map.retrieve_with_prefix(prefix);
for (const auto& result : lookup) {
symbols.push_back(result);
@@ -289,6 +286,10 @@ std::vector<std::shared_ptr<SymbolInfo>> SymbolInfoMap::lookup_symbols_starting_
return symbols;
}
std::vector<SymbolInfo*> SymbolInfoMap::get_all_symbols() const {
return m_symbol_map.get_all_elements();
}
std::set<std::string> SymbolInfoMap::lookup_names_starting_with(const std::string& prefix) const {
std::set<std::string> names;
const auto lookup = m_symbol_map.retrieve_with_prefix(prefix);
@@ -302,15 +303,11 @@ int SymbolInfoMap::symbol_count() const {
return m_symbol_map.size();
}
std::vector<std::shared_ptr<SymbolInfo>> SymbolInfoMap::get_all_symbols() const {
return m_symbol_map.get_all_elements();
}
void SymbolInfoMap::evict_symbols_using_file_index(const std::string& file_path) {
const auto standardized_path = file_util::convert_to_unix_path_separators(file_path);
if (m_file_symbol_index.find(standardized_path) != m_file_symbol_index.end()) {
for (const auto& symbol : m_file_symbol_index.at(standardized_path)) {
m_symbol_map.remove(symbol);
m_symbol_map.remove(symbol->m_name, symbol);
}
m_file_symbol_index.erase(standardized_path);
}
+8 -10
View File
@@ -6,7 +6,7 @@
#include "common/goos/Object.h"
#include "common/util/Assert.h"
#include "common/util/trie_map.h"
#include "common/util/trie_with_duplicates.h"
#include "goalc/compiler/Val.h"
@@ -120,13 +120,13 @@ struct SymbolInfo {
*/
class SymbolInfoMap {
goos::TextDb* m_textdb;
TrieMap<SymbolInfo> m_symbol_map;
TrieWithDuplicates<SymbolInfo> m_symbol_map;
// Indexes references to symbols by the file they are defined within
// This allows us to not only efficiently retrieve symbols by file, but also allows us to
// cleanup symbols when files are re-compiled.
std::unordered_map<std::string, std::vector<std::shared_ptr<SymbolInfo>>> m_file_symbol_index;
std::unordered_map<std::string, std::vector<SymbolInfo*>> m_file_symbol_index;
void add_symbol_to_file_index(const std::string& file_path, std::shared_ptr<SymbolInfo> symbol);
void add_symbol_to_file_index(const std::string& file_path, SymbolInfo* symbol);
public:
SymbolInfoMap(goos::TextDb* textdb) : m_textdb(textdb) {}
@@ -156,14 +156,12 @@ class SymbolInfoMap {
const std::vector<GoalArg>& args,
const MethodInfo& method_info,
const goos::Object& defining_form);
std::vector<std::shared_ptr<SymbolInfo>> lookup_symbols_by_file(
const std::string& file_path) const;
std::vector<std::shared_ptr<SymbolInfo>> lookup_exact_name(const std::string& name) const;
std::vector<std::shared_ptr<SymbolInfo>> lookup_symbols_starting_with(
const std::string& prefix) const;
std::vector<SymbolInfo*> lookup_symbols_by_file(const std::string& file_path) const;
std::vector<SymbolInfo*> lookup_exact_name(const std::string& name) const;
std::vector<SymbolInfo*> lookup_symbols_starting_with(const std::string& prefix) const;
std::set<std::string> lookup_names_starting_with(const std::string& prefix) const;
std::vector<SymbolInfo*> get_all_symbols() const;
int symbol_count() const;
std::vector<std::shared_ptr<SymbolInfo>> get_all_symbols() const;
// Uses the per-file index to find and evict symbols globally
// This should be done before re-compiling a file, symbols will be re-added to the DB if they are
// found again
-1
View File
@@ -8,7 +8,6 @@
#include "common/util/diff.h"
#include "common/util/string_util.h"
#include "common/util/term_util.h"
#include "common/util/trie_map.h"
#include "common/util/unicode_util.h"
#include "common/versions/versions.h"
+5 -6
View File
@@ -97,7 +97,7 @@ std::optional<GameVersion> Workspace::determine_game_version_from_uri(
return {};
}
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> Workspace::get_symbols_starting_with(
std::vector<symbol_info::SymbolInfo*> Workspace::get_symbols_starting_with(
const GameVersion game_version,
const std::string& symbol_prefix) {
if (m_compiler_instances.find(game_version) == m_compiler_instances.end()) {
@@ -109,7 +109,7 @@ std::vector<std::shared_ptr<symbol_info::SymbolInfo>> Workspace::get_symbols_sta
return compiler->lookup_symbol_info_by_prefix(symbol_prefix);
}
std::optional<std::shared_ptr<symbol_info::SymbolInfo>> Workspace::get_global_symbol_info(
std::optional<symbol_info::SymbolInfo*> Workspace::get_global_symbol_info(
const WorkspaceOGFile& file,
const std::string& symbol_name) {
if (m_compiler_instances.find(file.m_game_version) == m_compiler_instances.end()) {
@@ -154,7 +154,7 @@ std::optional<std::pair<TypeSpec, Type*>> Workspace::get_symbol_typeinfo(
std::optional<symbol_info::DefinitionLocation> Workspace::get_symbol_def_location(
const WorkspaceOGFile& file,
const std::shared_ptr<symbol_info::SymbolInfo> symbol_info) {
const symbol_info::SymbolInfo* symbol_info) {
const auto& def_loc = symbol_info->m_def_location;
if (!def_loc) {
return {};
@@ -181,7 +181,7 @@ Workspace::get_symbols_parent_type_path(const std::string& symbol_name,
if (symbol_infos.empty()) {
continue;
}
std::shared_ptr<symbol_info::SymbolInfo> symbol_info;
symbol_info::SymbolInfo* symbol_info;
if (symbol_infos.size() > 1) {
for (const auto& info : symbol_infos) {
if (info->m_kind == symbol_info::Kind::TYPE) {
@@ -415,8 +415,7 @@ void WorkspaceOGFile::parse_content(const std::string& content) {
ts_parser_delete(parser);
}
void WorkspaceOGFile::update_symbols(
const std::vector<std::shared_ptr<symbol_info::SymbolInfo>>& symbol_infos) {
void WorkspaceOGFile::update_symbols(const std::vector<symbol_info::SymbolInfo*>& symbol_infos) {
m_symbols.clear();
// TODO - sorting by definition location would be nice (maybe VSCode already does this?)
for (const auto& symbol_info : symbol_infos) {
+6 -8
View File
@@ -50,7 +50,7 @@ class WorkspaceOGFile {
std::vector<LSPSpec::Diagnostic> m_diagnostics;
void parse_content(const std::string& new_content);
void update_symbols(const std::vector<std::shared_ptr<symbol_info::SymbolInfo>>& symbol_infos);
void update_symbols(const std::vector<symbol_info::SymbolInfo*>& symbol_infos);
std::optional<std::string> get_symbol_at_position(const LSPSpec::Position position) const;
std::vector<OpenGOALFormResult> search_for_forms_that_begin_with(
std::vector<std::string> prefix) const;
@@ -139,17 +139,15 @@ class Workspace {
std::optional<DefinitionMetadata> get_definition_info_from_all_types(
const std::string& symbol_name,
const LSPSpec::DocumentUri& all_types_uri);
std::vector<std::shared_ptr<symbol_info::SymbolInfo>> get_symbols_starting_with(
const GameVersion game_version,
const std::string& symbol_prefix);
std::optional<std::shared_ptr<symbol_info::SymbolInfo>> get_global_symbol_info(
const WorkspaceOGFile& file,
const std::string& symbol_name);
std::vector<symbol_info::SymbolInfo*> get_symbols_starting_with(const GameVersion game_version,
const std::string& symbol_prefix);
std::optional<symbol_info::SymbolInfo*> get_global_symbol_info(const WorkspaceOGFile& file,
const std::string& symbol_name);
std::optional<std::pair<TypeSpec, Type*>> get_symbol_typeinfo(const WorkspaceOGFile& file,
const std::string& symbol_name);
std::optional<symbol_info::DefinitionLocation> get_symbol_def_location(
const WorkspaceOGFile& file,
const std::shared_ptr<symbol_info::SymbolInfo> symbol_info);
const symbol_info::SymbolInfo* symbol_info);
std::vector<std::tuple<std::string, std::string, Docs::DefinitionLocation>>
get_symbols_parent_type_path(const std::string& symbol_name, const GameVersion game_version);
std::vector<std::tuple<std::string, std::string, Docs::DefinitionLocation>> get_types_subtypes(