Rework Core ArcTable towards better memory efficiency and performance #537

HLWeil · 2025-07-09T14:13:10Z

These changes were necessary for much better memory efficiency, allowing Swate to depict and edit much bigger tables.

In principle this was done in two parts:

The ArcTable now contains a "ValueMap", which maps from the cell Hash to the actual CompositeCell. Were previously the CompositeCell was stored we now store this Hash, reducing memory usage for Cells appearing repreadetly in the same table
Instead of a 2D canvas, we now store the Table as columns, where a column may also be represented by a constant value. For parameters, it is very often the case that we have a column with only one distinct value. In this case we just need to store it once.

Calling functions were adjusted accordingly

Freymaurer · 2025-07-14T07:43:57Z

src/Core/OntologyAnnotation.fs

@@ -4,7 +4,7 @@ open ARCtrl.Helper
 open Fable.Core

 [<AttachMembers>]
-type OntologyAnnotation(?name,?tsr,?tan, ?comments) =
+type OntologyAnnotation(?name,?tsr,?tan, ?tanInfo, ?comments) =


uhh thats a new one! What does it mean?

Freymaurer · 2025-07-14T07:45:27Z

src/Core/OntologyAnnotation.fs

@@ -19,17 +19,30 @@ type OntologyAnnotation(?name,?tsr,?tan, ?comments) =
        | tan -> tan
    let mutable _comments : ResizeArray<Comment> = defaultArg comments <| ResizeArray()

+    let mutable _tanInfo : {| IDSpace : string; LocalID : string |} option =


ohh i see what it is. But isn't this bad practise? Say we add a tsr and tan then tanInfo gets auto completed?

Why do we need this inside the constructor instead of a GET only member?

It's not my preferred solution either, but the TanInfo is relatively expensive to compute but quite important as a basis for the HashCode.
By allowing this info to be set from the outside we can omit multiple computations of this value in some cases.

But then there must be clear indication what takes priority over which and how it behaves. say i use the following:

OntologyAnnotation(name = "test",tsr = "TEST",tan = "00001", tanInfo = {|IDSpace = "TST"; LocalID = "22222"|})

By allowing this info to be set from the outside we can omit multiple computations of this value in some cases.

And can you give me an example for that, because currently i do not really see it

Freymaurer · 2025-07-14T08:05:07Z

src/Core/Table/ArcTable.fs

-    /// column * row index
+        with get() = _headers
+        and set(newHeaders) =
+            // SanityChecks.validate newHeaders values true |> ignore // TODO


Is this meant to stay "TODO"?

Nope, fixing

Freymaurer · 2025-07-14T08:17:40Z

src/Core/Table/ArcTable.fs

@@ -372,42 +391,49 @@ type ArcTable(name: string, headers: ResizeArray<CompositeHeader>, values: Syste

    // - Column API - //
    // GetColumnAt?
-    member this.GetColumn(columnIndex:int) : CompositeColumn =
+    member this.GetColumn(columnIndex:int, ?fillDefault : bool) : CompositeColumn =


fillDefault behaves different to my expectations. Did you discuss this with someone else in the department and decided that we fail if a cell does not exist inside row/column count? I thought we only fail for trying to access outside these params?

I mean, with the fillDefault flag every user can decide. I could add some comment to make it more clear. Or would you wish to reverse the default behavious, and have a failIfMissing flag instead?

I would completly remove this flag. Just return the filled columns, then check if the array contains empty cells if you want.

In my opinion it makes using the function more obscure.

Do you have a valid usecase for this?

And one more thing. F# 9 does increase f# support for null. This could allow us to fill missing cells with null, making its behavior closer to js and python while also increase computation time

Freymaurer · 2025-07-14T08:18:20Z

src/Core/Table/ArcTable.fs

-                | Some c -> c
-        |]
+        let cells =
+            [|


This can be optimized by first checking if columnIndex points to a Constant columnRef, then we can just fill with the same value

Freymaurer · 2025-07-14T08:20:33Z

src/Core/Table/ArcTable.fs

        let index = defaultArg index this.RowCount
-        let cells = 
+        let cells =


Again this behaves different then my expecatation. Did we not decide on a sparse approach?

Will wait for your answers before continuing the review.

True that, this should just increase the rowCount of the "Values"

Freymaurer · 2025-07-17T07:27:01Z

src/Core/OntologyAnnotation.fs

@@ -19,17 +19,30 @@ type OntologyAnnotation(?name,?tsr,?tan, ?comments) =
        | tan -> tan
    let mutable _comments : ResizeArray<Comment> = defaultArg comments <| ResizeArray()

+    let mutable _tanInfo : {| IDSpace : string; LocalID : string |} option =


But then there must be clear indication what takes priority over which and how it behaves. say i use the following:

OntologyAnnotation(name = "test",tsr = "TEST",tan = "00001", tanInfo = {|IDSpace = "TST"; LocalID = "22222"|})

Freymaurer · 2025-07-17T07:29:10Z

src/Core/OntologyAnnotation.fs

@@ -19,17 +19,30 @@ type OntologyAnnotation(?name,?tsr,?tan, ?comments) =
        | tan -> tan
    let mutable _comments : ResizeArray<Comment> = defaultArg comments <| ResizeArray()

+    let mutable _tanInfo : {| IDSpace : string; LocalID : string |} option =


By allowing this info to be set from the outside we can omit multiple computations of this value in some cases.

And can you give me an example for that, because currently i do not really see it

Freymaurer · 2025-07-17T07:30:30Z

src/Core/Table/ArcTable.fs

-    /// column * row index
+        with get() = _headers
+        and set(newHeaders) =
+            SanityChecks.validateArcTableValues newHeaders _values true |> ignore // TODO


why the //TODO

Freymaurer · 2025-07-17T07:32:05Z

src/Core/Table/ArcTable.fs

+    member this.RowCount
+        with get() = _values.RowCount
+        and set (newRowCount) =
+            _values.RowCount <- newRowCount


if we set a rowCount we must delete all cells below that rowcount?

Say we have a cell at row 20 and we set row <- 10, then we must delete cell at row 20?

Freymaurer · 2025-07-17T07:33:38Z

src/Core/Table/ArcTable.fs

+        and set (newRowCount) =
+            _values.RowCount <- newRowCount
+
+    static member getRowCount (table:ArcTable) = table.RowCount


member this.ColumnCount and static member columnCount (table:ArcTable), then why do we have member this.RowCount and static member getRowCount (table:ArcTable)

Freymaurer · 2025-07-17T08:48:42Z

src/Core/Table/ArcTableAux.fs

-            | None                              -> CompositeCell.emptyTerm
-            | Some (CompositeCell.Unitized _)   -> CompositeCell.emptyUnitized
-            | _                                 -> failwith "[extendBodyCells] This should never happen, IsTermColumn header must be paired with either term or unitized cell."
+    let removeRowCells_withIndexChange (rowIndex:int) (values:ArcTableValues) =


might be good to have a function like this for multiple, this would increase performance for "RemoveRows" by quite a bit

Freymaurer · 2025-07-17T08:50:36Z

src/Core/Table/ArcTableAux.fs

        /// Therefore we need to start with the last column to not overwrite any values we still need to shift
        let increaseColumnIndices() =
            /// Get last column index
            let lastColumnIndex = System.Math.Max(startColCount - 1, 0) // If there are no columns. We get negative last column index. In this case just return 0.
            // start with last column index and go down to `index`
            for columnIndex = lastColumnIndex downto index do
-                for rowIndex in 0 .. startRowCount do
-                    moveCellTo(columnIndex,rowIndex,columnIndex+numberOfNewColumns,rowIndex) values
+                moveColumnCellsTo (columnIndex) (columnIndex + numberOfNewColumns) values
        /// Then we can set the new column at `index`
        let setNewCells() =
            // Not sure if this is intended? If we for example `forceReplace` a single column table with `Input`and 5 rows with a new column of `Input` ..


i think this comment is outdated to the previous model

Freymaurer · 2025-07-17T08:51:11Z

src/Core/Table/ArcTableAux.fs

-                setCellAt (columnIndex,rowIndex,lastValue) values
-
-    let addRow (index:int) (newCells:CompositeCell []) (headers: ResizeArray<CompositeHeader>) (values:Dictionary<int*int,CompositeCell>) =
+    let fillMissingCells (headers: ResizeArray<CompositeHeader>) (values:ArcTableValues) =


this can be deleted? Where do we use this?

I still kept the function, it will fill in default cells like before. Might be interesting in cases where we expect to make many random access calls and want to evaluate default values beforehand. Not used by any of our own functions.

Freymaurer · 2025-07-17T08:54:37Z

src/Core/Table/ArcTables.fs

-        |> Seq.toArray
-        |> Array.collect ArcTable.SplitByProtocolREF
-        |> Array.map (fun t ->
+        |> ResizeArray


rmv? ResizeArray.collect should work on a seq and returns an resizearray?

Freymaurer · 2025-07-17T08:56:03Z

src/Json/Decode.fs

+
+
+    let dictionary (keyDecoder : Decoder<'key>) (valueDecoder : Decoder<'value>) : Decoder<Dictionary<'key,'value>> =


i have no idea what happens here, trust!

Related Work Items: #5

Freymaurer and others added 24 commits June 16, 2025 16:59

Start working on ArcTable redesign ✨

0651f02

continue reworking arcTable by adding ArcTableValues type

d3f27b1

finish up first version of ArcTableValues type

179bcdb

finish up first version of reworked ArcTable

233344d

start reworking ArcTable references

df14ca0

finish up first version of ArcTable rework in source code

10671d5

adjust tests according to ArcTable rework

3711615

some fixes on ArcTable against tests

fd94ddb

small fixes against tests for ArcTable rework

a3eec49

some fixes to ArcTable according to tests

214160c

fix arctable cell addition and retrieval according to tests

be45cbc

fix some ArcTable tests

a8f5c5e

implement fillmissingcells

913ee6e

change OntologyAnnotation TanInfo computation time

0713287

improve performance of Annotation Table Read

5213ffd

fix final tests against dotnet for table rework

be5876b

remove erase from ColumnRefValues to fix js and py

0895fd1

small fixes to ArcTable functions

585017d

heavily improve performance of ArcTable addRow and addColumn

86e2a22

small fix to ArcTable

4ff9bc4

update default ArcTable json parsing

7be7abf

add and run performance tests

8f127ba

update ArcTable compressed json parsing

8bdd981

adjust ArcTable json parsing according to changes in ArcTable structure

02efde4

HLWeil requested a review from Freymaurer July 9, 2025 14:13

extend and run performance tests

0a69048

Freymaurer reviewed Jul 14, 2025

View reviewed changes

HLWeil added 2 commits July 15, 2025 16:35

small performance improvement to ArcTable.GetColumn

70ccc25

small fixes to ArcTable according to PR comments

3e30224

Freymaurer requested changes Jul 17, 2025

View reviewed changes

HLWeil added 3 commits July 21, 2025 09:08

continue reworking ArcTable according to change requests in #537

995e668

Related Work Items: #5

continue reworking ArcTable according to change requests in #537

589322a

add preliminary removeRowRange function

5fcfda1

Freymaurer approved these changes Jul 21, 2025

View reviewed changes

HLWeil merged commit 3dcc574 into main Jul 21, 2025
2 checks passed



		let dictionary (keyDecoder : Decoder<'key>) (valueDecoder : Decoder<'value>) : Decoder<Dictionary<'key,'value>> =

Rework Core ArcTable towards better memory efficiency and performance #537

Rework Core ArcTable towards better memory efficiency and performance #537

Uh oh!

Conversation

HLWeil commented Jul 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!