Add an allocator to store the symbol names.

This CL adds an allocator, owned by the SymbolTable, which stores the
names of all the symbols in the table. The Symbols then have a
`string_view` to their name.

Change-Id: I28e5b2aefcf9f67c1877b7ebab52416f780bd8c6
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/127300
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>

CMakeLists.txt

@@ -172,7 +172,6 @@ option_if_not_defined(TINT_BUILD_REMOTE_COMPILE "Build the remote-compile tool f
 
 option_if_not_defined(TINT_ENABLE_BREAK_IN_DEBUGGER "Enable tint::debugger::Break()" OFF)
 option_if_not_defined(TINT_CHECK_CHROMIUM_STYLE "Check for [chromium-style] issues during build" OFF)
-option_if_not_defined(TINT_SYMBOL_STORE_DEBUG_NAME "Enable storing of name in tint::ast::Symbol to help debugging the AST" OFF)
 option_if_not_defined(TINT_RANDOMIZE_HASHES "Randomize the hash seed value to detect non-deterministic output" OFF)
 
 # Recommended setting for compability with future abseil releases.

src/tint/CMakeLists.txt

@@ -786,9 +786,6 @@ target_link_libraries(tint_val tint_utils_io)
 add_library(libtint ${TINT_LIB_SRCS})
 tint_default_compile_options(libtint)
 target_link_libraries(libtint tint_diagnostic_utils absl_strings)
-if (${TINT_SYMBOL_STORE_DEBUG_NAME})
-    target_compile_definitions(libtint PUBLIC "TINT_SYMBOL_STORE_DEBUG_NAME=1")
-endif()
 set_target_properties(libtint PROPERTIES OUTPUT_NAME "tint")
 
 if (${TINT_BUILD_FUZZERS})

src/tint/ast/identifier_expression_test.cc

@@ -24,12 +24,12 @@ using IdentifierExpressionTest = TestHelper;
 
 TEST_F(IdentifierExpressionTest, Creation) {
     auto* i = Expr("ident");
-    EXPECT_EQ(i->identifier->symbol, Symbol(1, ID()));
+    EXPECT_EQ(i->identifier->symbol, Symbol(1, ID(), "ident"));
 }
 
 TEST_F(IdentifierExpressionTest, CreationTemplated) {
     auto* i = Expr(Ident("ident", true));
-    EXPECT_EQ(i->identifier->symbol, Symbol(1, ID()));
+    EXPECT_EQ(i->identifier->symbol, Symbol(1, ID(), "ident"));
     auto* tmpl_ident = i->identifier->As<TemplatedIdentifier>();
     ASSERT_NE(tmpl_ident, nullptr);
     EXPECT_EQ(tmpl_ident->arguments.Length(), 1_u);
@@ -38,7 +38,7 @@ TEST_F(IdentifierExpressionTest, CreationTemplated) {
 
 TEST_F(IdentifierExpressionTest, Creation_WithSource) {
     auto* i = Expr(Source{{20, 2}}, "ident");
-    EXPECT_EQ(i->identifier->symbol, Symbol(1, ID()));
+    EXPECT_EQ(i->identifier->symbol, Symbol(1, ID(), "ident"));
 
     EXPECT_EQ(i->source.range, (Source::Range{{20, 2}}));
     EXPECT_EQ(i->identifier->source.range, (Source::Range{{20, 2}}));

src/tint/ast/identifier_test.cc

@@ -22,12 +22,12 @@ using IdentifierTest = TestHelper;
 
 TEST_F(IdentifierTest, Creation) {
     auto* i = Ident("ident");
-    EXPECT_EQ(i->symbol, Symbol(1, ID()));
+    EXPECT_EQ(i->symbol, Symbol(1, ID(), "ident"));
 }
 
 TEST_F(IdentifierTest, Creation_WithSource) {
     auto* i = Ident(Source{{20, 2}}, "ident");
-    EXPECT_EQ(i->symbol, Symbol(1, ID()));
+    EXPECT_EQ(i->symbol, Symbol(1, ID(), "ident"));
 
     auto src = i->source;
     EXPECT_EQ(src.range.begin.line, 20u);

src/tint/clone_context.cc

@@ -33,7 +33,7 @@ CloneContext::CloneContext(ProgramBuilder* to, Program const* from, bool auto_cl
         // Almost all transforms will want to clone all symbols before doing any
         // work, to avoid any newly created symbols clashing with existing symbols
         // in the source program and causing them to be renamed.
-        from->Symbols().Foreach([&](Symbol s, const std::string&) { Clone(s); });
+        from->Symbols().Foreach([&](Symbol s) { Clone(s); });
     }
 }
 

src/tint/program_builder.cc

@@ -76,7 +76,7 @@ ProgramBuilder ProgramBuilder::Wrap(const Program* program) {
     builder.ast_ =
         builder.create<ast::Module>(program->AST().source, program->AST().GlobalDeclarations());
     builder.sem_ = sem::Info::Wrap(program->Sem());
-    builder.symbols_ = program->Symbols();
+    builder.symbols_.Wrap(program->Symbols());
     builder.diagnostics_ = program->Diagnostics();
     return builder;
 }

src/tint/reader/spirv/parser_type_test.cc

@@ -21,7 +21,7 @@ namespace tint::reader::spirv {
 namespace {
 
 TEST(SpvParserTypeTest, SameArgumentsGivesSamePointer) {
-    Symbol sym(Symbol(1, {}));
+    Symbol sym(Symbol(1, {}, "1"));
 
     TypeManager ty;
     EXPECT_EQ(ty.Void(), ty.Void());
@@ -50,8 +50,8 @@ TEST(SpvParserTypeTest, SameArgumentsGivesSamePointer) {
 }
 
 TEST(SpvParserTypeTest, DifferentArgumentsGivesDifferentPointer) {
-    Symbol sym_a(Symbol(1, {}));
+    Symbol sym_a(Symbol(1, {}, "1"));
-    Symbol sym_b(Symbol(2, {}));
+    Symbol sym_b(Symbol(2, {}, "2"));
 
     TypeManager ty;
     EXPECT_NE(ty.Pointer(ty.I32(), builtin::AddressSpace::kUndefined),

src/tint/scope_stack_test.cc

@@ -23,8 +23,8 @@ class ScopeStackTest : public ProgramBuilder, public testing::Test {};
 
 TEST_F(ScopeStackTest, Get) {
     ScopeStack<Symbol, uint32_t> s;
-    Symbol a(1, ID());
+    Symbol a(1, ID(), "1");
-    Symbol b(3, ID());
+    Symbol b(3, ID(), "3");
     s.Push();
     s.Set(a, 5u);
     s.Set(b, 10u);
@@ -45,14 +45,14 @@ TEST_F(ScopeStackTest, Get) {
 
 TEST_F(ScopeStackTest, Get_MissingSymbol) {
     ScopeStack<Symbol, uint32_t> s;
-    Symbol sym(1, ID());
+    Symbol sym(1, ID(), "1");
     EXPECT_EQ(s.Get(sym), 0u);
 }
 
 TEST_F(ScopeStackTest, Set) {
     ScopeStack<Symbol, uint32_t> s;
-    Symbol a(1, ID());
+    Symbol a(1, ID(), "1");
-    Symbol b(2, ID());
+    Symbol b(2, ID(), "2");
 
     EXPECT_EQ(s.Set(a, 5u), 0u);
     EXPECT_EQ(s.Get(a), 5u);
@@ -69,8 +69,8 @@ TEST_F(ScopeStackTest, Set) {
 
 TEST_F(ScopeStackTest, Clear) {
     ScopeStack<Symbol, uint32_t> s;
-    Symbol a(1, ID());
+    Symbol a(1, ID(), "1");
-    Symbol b(2, ID());
+    Symbol b(2, ID(), "2");
 
     EXPECT_EQ(s.Set(a, 5u), 0u);
     EXPECT_EQ(s.Get(a), 5u);

src/tint/symbol.cc

@@ -20,12 +20,8 @@ namespace tint {
 
 Symbol::Symbol() = default;
 
-Symbol::Symbol(uint32_t val, tint::ProgramID program_id) : val_(val), program_id_(program_id) {}
+Symbol::Symbol(uint32_t val, tint::ProgramID pid, std::string_view name)
-
+    : val_(val), program_id_(pid), name_(name) {}
-#if TINT_SYMBOL_STORE_DEBUG_NAME
-Symbol::Symbol(uint32_t val, tint::ProgramID pid, std::string debug_name)
-    : val_(val), program_id_(pid), debug_name_(std::move(debug_name)) {}
-#endif
 
 Symbol::Symbol(const Symbol& o) = default;
 
@@ -56,4 +52,8 @@ std::string Symbol::to_str() const {
     return "$" + std::to_string(val_);
 }
 
+std::string Symbol::Name() const {
+    return std::string(name_);
+}
+
 }  // namespace tint

src/tint/symbol.h

@@ -15,13 +15,6 @@
 #ifndef SRC_TINT_SYMBOL_H_
 #define SRC_TINT_SYMBOL_H_
 
-// If TINT_SYMBOL_STORE_DEBUG_NAME is 1, Symbol instances store a `debug_name_`
-// member initialized with the name of the identifier they represent. This
-// member is not exposed, but is useful for debugging purposes.
-#ifndef TINT_SYMBOL_STORE_DEBUG_NAME
-#define TINT_SYMBOL_STORE_DEBUG_NAME 0
-#endif
-
 #include <string>
 
 #include "src/tint/program_id.h"
@@ -34,17 +27,12 @@ class Symbol {
     /// Constructor
     /// An invalid symbol
     Symbol();
-    /// Constructor
-    /// @param val the symbol value
-    /// @param program_id the identifier of the program that owns this Symbol
-    Symbol(uint32_t val, tint::ProgramID program_id);
-#if TINT_SYMBOL_STORE_DEBUG_NAME
     /// Constructor
     /// @param val the symbol value
     /// @param pid the identifier of the program that owns this Symbol
-    /// @param debug_name name of symbols used only for debugging
+    /// @param name the name this symbol represents
-    Symbol(uint32_t val, tint::ProgramID pid, std::string debug_name);
+    Symbol(uint32_t val, tint::ProgramID pid, std::string_view name);
-#endif
+
     /// Copy constructor
     /// @param o the symbol to copy
     Symbol(const Symbol& o);
@@ -88,15 +76,17 @@ class Symbol {
     /// @return the string representation of the symbol
     std::string to_str() const;
 
+    /// Converts the symbol to the registered name
+    /// @returns the string representing the name of the symbol
+    std::string Name() const;
+
     /// @returns the identifier of the Program that owns this symbol.
     tint::ProgramID ProgramID() const { return program_id_; }
 
   private:
     uint32_t val_ = static_cast<uint32_t>(-1);
     tint::ProgramID program_id_;
-#if TINT_SYMBOL_STORE_DEBUG_NAME
+    std::string_view name_;
-    std::string debug_name_;
-#endif
 };
 
 /// @param sym the Symbol

src/tint/symbol_table.cc

@@ -20,14 +20,10 @@ namespace tint {
 
 SymbolTable::SymbolTable(tint::ProgramID program_id) : program_id_(program_id) {}
 
-SymbolTable::SymbolTable(const SymbolTable&) = default;
-
 SymbolTable::SymbolTable(SymbolTable&&) = default;
 
 SymbolTable::~SymbolTable() = default;
 
-SymbolTable& SymbolTable::operator=(const SymbolTable& other) = default;
-
 SymbolTable& SymbolTable::operator=(SymbolTable&&) = default;
 
 Symbol SymbolTable::Register(const std::string& name) {
@@ -38,15 +34,19 @@ Symbol SymbolTable::Register(const std::string& name) {
         return *it;
     }
 
-#if TINT_SYMBOL_STORE_DEBUG_NAME
+    char* name_mem = name_allocator_.Allocate(name.length() + 1);
-    Symbol sym(next_symbol_, program_id_, name);
+    if (name_mem == nullptr) {
-#else
+        return Symbol();
-    Symbol sym(next_symbol_, program_id_);
+    }
-#endif
+
+    memcpy(name_mem, name.data(), name.length() + 1);
+    std::string_view nv(name_mem, name.length());
+
+    Symbol sym(next_symbol_, program_id_, nv);
+
     ++next_symbol_;
 
-    name_to_symbol_.Add(name, sym);
+    name_to_symbol_.Add(name_mem, sym);
-    symbol_to_name_.Add(sym, name);
 
     return sym;
 }
@@ -58,12 +58,7 @@ Symbol SymbolTable::Get(const std::string& name) const {
 
 std::string SymbolTable::NameFor(const Symbol symbol) const {
     TINT_ASSERT_PROGRAM_IDS_EQUAL(Symbol, program_id_, symbol);
-    auto it = symbol_to_name_.Find(symbol);
+    return symbol.Name();
-    if (!it) {
-        return symbol.to_str();
-    }
-
-    return *it;
 }
 
 Symbol SymbolTable::New(std::string prefix /* = "" */) {

src/tint/symbol_table.h

@@ -16,9 +16,10 @@
 #define SRC_TINT_SYMBOL_TABLE_H_
 
 #include <string>
-#include "utils/hashmap.h"
 
 #include "src/tint/symbol.h"
+#include "utils/bump_allocator.h"
+#include "utils/hashmap.h"
 
 namespace tint {
 
@@ -29,22 +30,29 @@ class SymbolTable {
     /// @param program_id the identifier of the program that owns this symbol
     /// table
     explicit SymbolTable(tint::ProgramID program_id);
-    /// Copy constructor
-    SymbolTable(const SymbolTable&);
     /// Move Constructor
     SymbolTable(SymbolTable&&);
     /// Destructor
     ~SymbolTable();
 
-    /// Copy assignment
-    /// @param other the symbol table to copy
-    /// @returns the new symbol table
-    SymbolTable& operator=(const SymbolTable& other);
     /// Move assignment
     /// @param other the symbol table to move
     /// @returns the symbol table
     SymbolTable& operator=(SymbolTable&& other);
 
+    /// Wrap sets this symbol table to hold symbols which point to the allocated names in @p o.
+    /// The symbol table after Wrap is intended to temporarily extend the objects
+    /// of an existing immutable SymbolTable
+    /// As the copied objects are owned by @p o, @p o must not be destructed
+    /// or assigned while using this symbol table.
+    /// @param o the immutable SymbolTable to extend
+    void Wrap(const SymbolTable& o) {
+        next_symbol_ = o.next_symbol_;
+        name_to_symbol_ = o.name_to_symbol_;
+        last_prefix_to_index_ = o.last_prefix_to_index_;
+        program_id_ = o.program_id_;
+    }
+
     /// Registers a name into the symbol table, returning the Symbol.
     /// @param name the name to register
     /// @returns the symbol representing the given name
@@ -70,11 +78,11 @@ class SymbolTable {
 
     /// Foreach calls the callback function `F` for each symbol in the table.
     /// @param callback must be a function or function-like object with the
-    /// signature: `void(Symbol, const std::string&)`
+    /// signature: `void(Symbol)`
     template <typename F>
     void Foreach(F&& callback) const {
-        for (auto it : symbol_to_name_) {
+        for (auto it : name_to_symbol_) {
-            callback(it.key, it.value);
+            callback(it.value);
         }
     }
 
@@ -82,13 +90,17 @@ class SymbolTable {
     tint::ProgramID ProgramID() const { return program_id_; }
 
   private:
+    SymbolTable(const SymbolTable&) = delete;
+    SymbolTable& operator=(const SymbolTable& other) = delete;
+
     // The value to be associated to the next registered symbol table entry.
     uint32_t next_symbol_ = 1;
 
-    utils::Hashmap<Symbol, std::string, 0> symbol_to_name_;
+    utils::Hashmap<std::string_view, Symbol, 0> name_to_symbol_;
-    utils::Hashmap<std::string, Symbol, 0> name_to_symbol_;
     utils::Hashmap<std::string, size_t, 0> last_prefix_to_index_;
     tint::ProgramID program_id_;
+
+    utils::BumpAllocator name_allocator_;
 };
 
 /// @param symbol_table the SymbolTable

src/tint/symbol_table_test.cc

@@ -24,16 +24,16 @@ using SymbolTableTest = testing::Test;
 TEST_F(SymbolTableTest, GeneratesSymbolForName) {
     auto program_id = ProgramID::New();
     SymbolTable s{program_id};
-    EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
+    EXPECT_EQ(Symbol(1, program_id, "name"), s.Register("name"));
-    EXPECT_EQ(Symbol(2, program_id), s.Register("another_name"));
+    EXPECT_EQ(Symbol(2, program_id, "another_name"), s.Register("another_name"));
 }
 
 TEST_F(SymbolTableTest, DeduplicatesNames) {
     auto program_id = ProgramID::New();
     SymbolTable s{program_id};
-    EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
+    EXPECT_EQ(Symbol(1, program_id, "name"), s.Register("name"));
-    EXPECT_EQ(Symbol(2, program_id), s.Register("another_name"));
+    EXPECT_EQ(Symbol(2, program_id, "another_name"), s.Register("another_name"));
-    EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
+    EXPECT_EQ(Symbol(1, program_id, "name"), s.Register("name"));
 }
 
 TEST_F(SymbolTableTest, ReturnsNameForSymbol) {
@@ -46,7 +46,7 @@ TEST_F(SymbolTableTest, ReturnsNameForSymbol) {
 TEST_F(SymbolTableTest, ReturnsBlankForMissingSymbol) {
     auto program_id = ProgramID::New();
     SymbolTable s{program_id};
-    EXPECT_EQ("$2", s.NameFor(Symbol(2, program_id)));
+    EXPECT_EQ("$2", s.NameFor(Symbol(2, program_id, "$2")));
 }
 
 TEST_F(SymbolTableTest, AssertsForBlankString) {

src/tint/symbol_test.cc

@@ -22,12 +22,12 @@ namespace {
 using SymbolTest = testing::Test;
 
 TEST_F(SymbolTest, ToStr) {
-    Symbol sym(1, ProgramID::New());
+    Symbol sym(1, ProgramID::New(), "");
     EXPECT_EQ("$1", sym.to_str());
 }
 
 TEST_F(SymbolTest, CopyAssign) {
-    Symbol sym1(1, ProgramID::New());
+    Symbol sym1(1, ProgramID::New(), "");
     Symbol sym2;
 
     EXPECT_FALSE(sym2.IsValid());
@@ -38,9 +38,9 @@ TEST_F(SymbolTest, CopyAssign) {
 
 TEST_F(SymbolTest, Comparison) {
     auto program_id = ProgramID::New();
-    Symbol sym1(1, program_id);
+    Symbol sym1(1, program_id, "1");
-    Symbol sym2(2, program_id);
+    Symbol sym2(2, program_id, "2");
-    Symbol sym3(1, program_id);
+    Symbol sym3(1, program_id, "3");
 
     EXPECT_TRUE(sym1 == sym3);
     EXPECT_FALSE(sym1 != sym3);

src/tint/tint.natvis

@@ -55,8 +55,7 @@
 	</Type>
 
 	<Type Name="tint::Symbol">
-		<!-- Requires TINT_SYMBOL_STORE_DEBUG_NAME defined to 1 -->
+		<DisplayString Optional="true">{name_,sb}</DisplayString>
-		<DisplayString Optional="true">{debug_name_,sb}</DisplayString>
 	</Type>
 
 	<!--=================================================================-->