Spreadsheet: Store the column index in a Position instead of its name

This will make constructing (and destructing) Positions a lot cheaper
(as it no longer needs to ref() and unref() a String).
Resulted from #5483, but doesn't fix it.

Userland/Applications/Spreadsheet/JSIntegration.cpp

@@ -125,7 +125,7 @@ JS::Value SheetGlobalObject::get(const JS::PropertyName& name, JS::Value receive
 
             return JS::js_undefined();
         }
-        if (auto pos = Sheet::parse_cell_name(name.as_string()); pos.has_value()) {
+        if (auto pos = m_sheet.parse_cell_name(name.as_string()); pos.has_value()) {
             auto& cell = m_sheet.ensure(pos.value());
             cell.reference_from(m_sheet.current_evaluated_cell());
             return cell.typed_js_data();
@@ -138,7 +138,7 @@ JS::Value SheetGlobalObject::get(const JS::PropertyName& name, JS::Value receive
 bool SheetGlobalObject::put(const JS::PropertyName& name, JS::Value value, JS::Value receiver)
 {
     if (name.is_string()) {
-        if (auto pos = Sheet::parse_cell_name(name.as_string()); pos.has_value()) {
+        if (auto pos = m_sheet.parse_cell_name(name.as_string()); pos.has_value()) {
             auto& cell = m_sheet.ensure(pos.value());
             if (auto current = m_sheet.current_evaluated_cell())
                 current->reference_from(&cell);
@@ -178,7 +178,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::get_real_cell_contents)
     if (!this_object)
         return JS::js_null();
 
-    if (StringView("SheetGlobalObject") != this_object->class_name()) {
+    if (!is<SheetGlobalObject>(this_object)) {
         vm.throw_exception<JS::TypeError>(global_object, JS::ErrorType::NotA, "SheetGlobalObject");
         return {};
     }
@@ -195,7 +195,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::get_real_cell_contents)
         vm.throw_exception<JS::TypeError>(global_object, "Expected a String argument to get_real_cell_contents()");
         return {};
     }
-    auto position = Sheet::parse_cell_name(name_value.as_string().string());
+    auto position = sheet_object->m_sheet.parse_cell_name(name_value.as_string().string());
     if (!position.has_value()) {
         vm.throw_exception<JS::TypeError>(global_object, "Invalid cell name");
         return {};
@@ -217,7 +217,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::set_real_cell_contents)
     if (!this_object)
         return JS::js_null();
 
-    if (StringView("SheetGlobalObject") != this_object->class_name()) {
+    if (!is<SheetGlobalObject>(this_object)) {
         vm.throw_exception<JS::TypeError>(global_object, JS::ErrorType::NotA, "SheetGlobalObject");
         return {};
     }
@@ -234,7 +234,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::set_real_cell_contents)
         vm.throw_exception<JS::TypeError>(global_object, "Expected the first argument of set_real_cell_contents() to be a String");
         return {};
     }
-    auto position = Sheet::parse_cell_name(name_value.as_string().string());
+    auto position = sheet_object->m_sheet.parse_cell_name(name_value.as_string().string());
     if (!position.has_value()) {
         vm.throw_exception<JS::TypeError>(global_object, "Invalid cell name");
         return {};
@@ -254,6 +254,17 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::set_real_cell_contents)
 
 JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::parse_cell_name)
 {
+    auto* this_object = vm.this_value(global_object).to_object(global_object);
+    if (!this_object)
+        return JS::js_null();
+
+    if (!is<SheetGlobalObject>(this_object)) {
+        vm.throw_exception<JS::TypeError>(global_object, JS::ErrorType::NotA, "SheetGlobalObject");
+        return {};
+    }
+
+    auto sheet_object = static_cast<SheetGlobalObject*>(this_object);
+
     if (vm.argument_count() != 1) {
         vm.throw_exception<JS::TypeError>(global_object, "Expected exactly one argument to parse_cell_name()");
         return {};
@@ -263,12 +274,12 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::parse_cell_name)
         vm.throw_exception<JS::TypeError>(global_object, "Expected a String argument to parse_cell_name()");
         return {};
     }
-    auto position = Sheet::parse_cell_name(name_value.as_string().string());
+    auto position = sheet_object->m_sheet.parse_cell_name(name_value.as_string().string());
     if (!position.has_value())
         return JS::js_undefined();
 
     auto object = JS::Object::create_empty(global_object);
-    object->put("column", JS::js_string(vm, position.value().column));
+    object->put("column", JS::js_string(vm, sheet_object->m_sheet.column(position.value().column)));
     object->put("row", JS::Value((unsigned)position.value().row));
 
     return object;
@@ -285,7 +296,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::current_cell_position)
     if (!this_object)
         return JS::js_null();
 
-    if (StringView("SheetGlobalObject") != this_object->class_name()) {
+    if (!is<SheetGlobalObject>(this_object)) {
         vm.throw_exception<JS::TypeError>(global_object, JS::ErrorType::NotA, "SheetGlobalObject");
         return {};
     }
@@ -298,7 +309,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::current_cell_position)
     auto position = current_cell->position();
 
     auto object = JS::Object::create_empty(global_object);
-    object->put("column", JS::js_string(vm, position.column));
+    object->put("column", JS::js_string(vm, sheet_object->m_sheet.column(position.column)));
     object->put("row", JS::Value((unsigned)position.row));
 
     return object;
@@ -323,7 +334,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::column_index)
     if (!this_object)
         return JS::js_null();
 
-    if (StringView("SheetGlobalObject") != this_object->class_name()) {
+    if (!is<SheetGlobalObject>(this_object)) {
         vm.throw_exception<JS::TypeError>(global_object, JS::ErrorType::NotA, "SheetGlobalObject");
         return {};
     }
@@ -364,7 +375,7 @@ JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::column_arithmetic)
     if (!this_object)
         return JS::js_null();
 
-    if (StringView("SheetGlobalObject") != this_object->class_name()) {
+    if (!is<SheetGlobalObject>(this_object)) {
         vm.throw_exception<JS::TypeError>(global_object, JS::ErrorType::NotA, "SheetGlobalObject");
         return {};
     }

Userland/Applications/Spreadsheet/Position.h

@@ -32,9 +32,25 @@
 
 namespace Spreadsheet {
 
+class Sheet;
+
 struct Position {
-    String column;
-    size_t row { 0 };
+    Position() = default;
+
+    Position(size_t column, size_t row)
+        : column(column)
+        , row(row)
+        , m_hash(pair_int_hash(column, row))
+    {
+    }
+
+    ALWAYS_INLINE u32 hash() const
+    {
+        if (m_hash == 0)
+            return m_hash = int_hash(column * 65537 + row);
+
+        return m_hash;
+    }
 
     bool operator==(const Position& other) const
     {
@@ -46,15 +62,13 @@ struct Position {
         return !(other == *this);
     }
 
-    URL to_url() const
-    {
-        URL url;
-        url.set_protocol("spreadsheet");
-        url.set_host("cell");
-        url.set_path(String::formatted("/{}", getpid()));
-        url.set_fragment(String::formatted("{}{}", column, row));
-        return url;
-    }
+    URL to_url(const Sheet& sheet) const;
+
+    size_t column { 0 };
+    size_t row { 0 };
+
+private:
+    mutable u32 m_hash { 0 };
 };
 
 }

Userland/Applications/Spreadsheet/Spreadsheet.cpp

@@ -233,7 +233,7 @@ Cell* Sheet::at(const Position& position)
     return it->value;
 }
 
-Optional<Position> Sheet::parse_cell_name(const StringView& name)
+Optional<Position> Sheet::parse_cell_name(const StringView& name) const
 {
     GenericLexer lexer(name);
     auto col = lexer.consume_while(isalpha);
@@ -242,7 +242,11 @@ Optional<Position> Sheet::parse_cell_name(const StringView& name)
     if (!lexer.is_eof() || row.is_empty() || col.is_empty())
         return {};
 
-    return Position { col, row.to_uint().value() };
+    auto it = m_columns.find(col);
+    if (it == m_columns.end())
+        return {};
+
+    return Position { it.index(), row.to_uint().value() };
 }
 
 Optional<size_t> Sheet::column_index(const StringView& column_name) const
@@ -302,27 +306,23 @@ Optional<Position> Sheet::position_from_url(const URL& url) const
 
 Position Sheet::offset_relative_to(const Position& base, const Position& offset, const Position& offset_base) const
 {
-    auto offset_column_it = m_columns.find(offset.column);
-    auto offset_base_column_it = m_columns.find(offset_base.column);
-    auto base_column_it = m_columns.find(base.column);
-
-    if (offset_column_it.is_end()) {
+    if (offset.column >= m_columns.size()) {
         dbgln("Column '{}' does not exist!", offset.column);
         return base;
     }
-    if (offset_base_column_it.is_end()) {
-        dbgln("Column '{}' does not exist!", offset.column);
+    if (offset_base.column >= m_columns.size()) {
+        dbgln("Column '{}' does not exist!", offset_base.column);
         return base;
     }
-    if (base_column_it.is_end()) {
-        dbgln("Column '{}' does not exist!", offset.column);
+    if (base.column >= m_columns.size()) {
+        dbgln("Column '{}' does not exist!", base.column);
         return offset;
     }
 
-    auto new_column = column(offset_column_it.index() + base_column_it.index() - offset_base_column_it.index());
+    auto new_column = offset.column + base.column - offset_base.column;
     auto new_row = offset.row + base.row - offset_base.row;
 
-    return { move(new_column), new_row };
+    return { new_column, new_row };
 }
 
 void Sheet::copy_cells(Vector<Position> from, Vector<Position> to, Optional<Position> resolve_relative_to)
@@ -353,7 +353,7 @@ void Sheet::copy_cells(Vector<Position> from, Vector<Position> to, Optional<Posi
         auto& target = to.first();
 
         for (auto& position : from) {
-            dbgln_if(COPY_DEBUG, "Paste from '{}' to '{}'", position.to_url(), target.to_url());
+            dbgln_if(COPY_DEBUG, "Paste from '{}' to '{}'", position.to_url(*this), target.to_url(*this));
             copy_to(position, resolve_relative_to.has_value() ? offset_relative_to(target, position, resolve_relative_to.value()) : target);
         }
 
@@ -364,7 +364,7 @@ void Sheet::copy_cells(Vector<Position> from, Vector<Position> to, Optional<Posi
         // Fill the target selection with the single cell.
         auto& source = from.first();
         for (auto& position : to) {
-            dbgln_if(COPY_DEBUG, "Paste from '{}' to '{}'", source.to_url(), position.to_url());
+            dbgln_if(COPY_DEBUG, "Paste from '{}' to '{}'", source.to_url(*this), position.to_url(*this));
             copy_to(source, resolve_relative_to.has_value() ? offset_relative_to(position, source, resolve_relative_to.value()) : position);
         }
         return;
@@ -411,7 +411,7 @@ RefPtr<Sheet> Sheet::from_json(const JsonObject& object, Workbook& workbook)
     };
 
     cells.for_each_member([&](auto& name, JsonValue& value) {
-        auto position_option = parse_cell_name(name);
+        auto position_option = sheet->parse_cell_name(name);
         if (!position_option.has_value())
             return IterationDecision::Continue;
 
@@ -600,7 +600,7 @@ Vector<Vector<String>> Sheet::to_xsv() const
     // First row = headers.
     size_t column_count = m_columns.size();
     if (columns_are_standard()) {
-        column_count = convert_from_string(bottom_right.column) + 1;
+        column_count = bottom_right.column + 1;
         Vector<String> cols;
         for (size_t i = 0; i < column_count; ++i)
             cols.append(m_columns[i]);
@@ -613,7 +613,7 @@ Vector<Vector<String>> Sheet::to_xsv() const
         Vector<String> row;
         row.resize(column_count);
         for (size_t j = 0; j < column_count; ++j) {
-            auto cell = at({ m_columns[j], i });
+            auto cell = at({ j, i });
             if (cell)
                 row[j] = cell->typed_display();
         }
@@ -643,7 +643,7 @@ RefPtr<Sheet> Sheet::from_xsv(const Reader::XSV& xsv, Workbook& workbook)
             auto str = row[i];
             if (str.is_empty())
                 continue;
-            Position position { cols[i], row.index() };
+            Position position { i, row.index() };
             auto cell = make<Cell>(str, position, *sheet);
             sheet->m_cells.set(position, move(cell));
         }
@@ -726,4 +726,14 @@ String Sheet::generate_inline_documentation_for(StringView function, size_t argu
     return builder.build();
 }
 
+URL Position::to_url(const Sheet& sheet) const
+{
+    URL url;
+    url.set_protocol("spreadsheet");
+    url.set_host("cell");
+    url.set_path(String::formatted("/{}", getpid()));
+    url.set_fragment(String::formatted("{}{}", sheet.column(column), row));
+    return url;
+}
+
 }

Userland/Applications/Spreadsheet/Spreadsheet.h

@@ -51,7 +51,7 @@ public:
 
     ~Sheet();
 
-    static Optional<Position> parse_cell_name(const StringView&);
+    Optional<Position> parse_cell_name(const StringView&) const;
     Optional<size_t> column_index(const StringView& column_name) const;
     Optional<String> column_arithmetic(const StringView& column_name, int offset);
 
@@ -179,9 +179,7 @@ struct Traits<Spreadsheet::Position> : public GenericTraits<Spreadsheet::Positio
     static constexpr bool is_trivial() { return false; }
     static unsigned hash(const Spreadsheet::Position& p)
     {
-        return pair_int_hash(
-            string_hash(p.column.characters(), p.column.length()),
-            u64_hash(p.row));
+        return p.hash();
     }
 };
 

Userland/Applications/Spreadsheet/SpreadsheetModel.cpp

@@ -43,7 +43,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
         return {};
 
     if (role == GUI::ModelRole::Display) {
-        const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() });
+        const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() });
         if (!cell)
             return String::empty();
 
@@ -73,10 +73,10 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
     }
 
     if (role == GUI::ModelRole::MimeData)
-        return Position { m_sheet->column(index.column()), (size_t)index.row() }.to_url().to_string();
+        return Position { (size_t)index.column(), (size_t)index.row() }.to_url(m_sheet).to_string();
 
     if (role == GUI::ModelRole::TextAlignment) {
-        const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() });
+        const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() });
         if (!cell)
             return {};
 
@@ -84,7 +84,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
     }
 
     if (role == GUI::ModelRole::ForegroundColor) {
-        const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() });
+        const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() });
         if (!cell)
             return {};
 
@@ -103,7 +103,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
     }
 
     if (role == GUI::ModelRole::BackgroundColor) {
-        const auto* cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() });
+        const auto* cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() });
         if (!cell)
             return {};
 
@@ -134,9 +134,9 @@ RefPtr<Core::MimeData> SheetModel::mime_data(const GUI::ModelSelection& selectio
 
     VERIFY(cursor);
 
-    Position cursor_position { m_sheet->column(cursor->column()), (size_t)cursor->row() };
+    Position cursor_position { (size_t)cursor->column(), (size_t)cursor->row() };
     auto new_data = String::formatted("{}\n{}",
-        cursor_position.to_url().to_string(),
+        cursor_position.to_url(m_sheet).to_string(),
         StringView(mime_data->data("text/x-spreadsheet-data")));
     mime_data->set_data("text/x-spreadsheet-data", new_data.to_byte_buffer());
 
@@ -164,7 +164,7 @@ void SheetModel::set_data(const GUI::ModelIndex& index, const GUI::Variant& valu
     if (!index.is_valid())
         return;
 
-    auto& cell = m_sheet->ensure({ m_sheet->column(index.column()), (size_t)index.row() });
+    auto& cell = m_sheet->ensure({ (size_t)index.column(), (size_t)index.row() });
     cell.set_data(value.to_string());
     update();
 }

Userland/Applications/Spreadsheet/SpreadsheetView.cpp

@@ -57,7 +57,7 @@ void SpreadsheetView::EditingDelegate::set_value(const GUI::Variant& value)
         return StringModelEditingDelegate::set_value(value);
 
     m_has_set_initial_value = true;
-    const auto option = m_sheet.at({ m_sheet.column(index().column()), (size_t)index().row() });
+    const auto option = m_sheet.at({ (size_t)index().column(), (size_t)index().row() });
     if (option)
         return StringModelEditingDelegate::set_value(option->source());
 
@@ -198,7 +198,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet)
     m_table_view->on_selection_change = [&] {
         m_sheet->selected_cells().clear();
         for (auto& index : m_table_view->selection().indexes()) {
-            Position position { m_sheet->column(index.column()), (size_t)index.row() };
+            Position position { (size_t)index.column(), (size_t)index.row() };
             m_sheet->selected_cells().set(position);
         }
 
@@ -208,7 +208,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet)
         Vector<Position> selected_positions;
         selected_positions.ensure_capacity(m_table_view->selection().size());
         for (auto& selection : m_table_view->selection().indexes())
-            selected_positions.empend(m_sheet->column(selection.column()), (size_t)selection.row());
+            selected_positions.empend((size_t)selection.column(), (size_t)selection.row());
 
         if (on_selection_changed) {
             on_selection_changed(move(selected_positions));
@@ -229,13 +229,13 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet)
     m_cell_range_context_menu->add_action(GUI::Action::create("Type and Formatting...", [this](auto&) {
         Vector<Position> positions;
         for (auto& index : m_table_view->selection().indexes()) {
-            Position position { m_sheet->column(index.column()), (size_t)index.row() };
+            Position position { (size_t)index.column(), (size_t)index.row() };
             positions.append(move(position));
         }
 
         if (positions.is_empty()) {
             auto& index = m_table_view->cursor_index();
-            Position position { m_sheet->column(index.column()), (size_t)index.row() };
+            Position position { (size_t)index.column(), (size_t)index.row() };
             positions.append(move(position));
         }
 
@@ -270,7 +270,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet)
             }
 
             // Drop always has a single target.
-            Position target { m_sheet->column(index.column()), (size_t)index.row() };
+            Position target { (size_t)index.column(), (size_t)index.row() };
             target_positions.append(move(target));
 
             if (source_positions.is_empty())
@@ -283,7 +283,7 @@ SpreadsheetView::SpreadsheetView(Sheet& sheet)
         }
 
         if (event.mime_data().has_text()) {
-            auto* target_cell = m_sheet->at({ m_sheet->column(index.column()), (size_t)index.row() });
+            auto* target_cell = m_sheet->at({ (size_t)index.column(), (size_t)index.row() });
             VERIFY(target_cell);
 
             target_cell->set_data(event.text());
@@ -304,7 +304,7 @@ void SpreadsheetView::show_event(GUI::ShowEvent&)
         Vector<Position> selected_positions;
         selected_positions.ensure_capacity(m_table_view->selection().size());
         for (auto& selection : m_table_view->selection().indexes())
-            selected_positions.empend(m_sheet->column(selection.column()), (size_t)selection.row());
+            selected_positions.empend((size_t)selection.column(), (size_t)selection.row());
 
         on_selection_changed(move(selected_positions));
     }

Userland/Applications/Spreadsheet/main.cpp

@@ -170,18 +170,18 @@ int main(int argc, char* argv[])
         bool first = true;
         auto cursor = spreadsheet_widget.current_selection_cursor();
         if (cursor) {
-            Spreadsheet::Position position { spreadsheet_widget.current_worksheet().column(cursor->column()), (size_t)cursor->row() };
-            url_builder.append(position.to_url().to_string());
+            Spreadsheet::Position position { (size_t)cursor->column(), (size_t)cursor->row() };
+            url_builder.append(position.to_url(spreadsheet_widget.current_worksheet()).to_string());
             url_builder.append('\n');
         }
 
         for (auto& cell : cells) {
             if (first && !cursor) {
-                url_builder.append(cell.to_url().to_string());
+                url_builder.append(cell.to_url(spreadsheet_widget.current_worksheet()).to_string());
                 url_builder.append('\n');
             }
 
-            url_builder.append(cell.to_url().to_string());
+            url_builder.append(cell.to_url(spreadsheet_widget.current_worksheet()).to_string());
             url_builder.append('\n');
 
             auto cell_data = spreadsheet_widget.current_worksheet().at(cell);