Isolate expressions per-context and ensure memory stability #1431

sc1f · 2021-05-26T10:12:01Z

This PR refactors the way that expressions are handled and stored in Perspective - previously, expressions columns were calculated and stored on the main table inside t_gstate and its intermediate tables (prev, current, delta, transitions, etc.). By having all expressions on the gnode's tables, expressions were "real" columns to the engine, and it guaranteed that all the features available to real columns (pivot, filter, etc.) were available to expression columns without having to do large refactors to the code that generated aggregates, pivots etc.

However, this meant that expressions were not "isolated" to the View they were created in - a user could not create a new expression named new column of float type, and then replace it immediately with an expression that returns a string, as the main table schema had already recorded new column as a float column and could not overwrite it.

Now, expression columns are stored per-context on data tables that only contain the expression columns belonging to the View. This means that each View can create expression columns that use the same aliases as other Views, and they can be of any type:

const v1 = await table.view({
  expressions: [`// new column\n "ask" - "bid"`]
});

const v2 = await table.view({
  expressions: [`// new column\n upper("client")`]
});

And now, expression columns are guaranteed to be deleted when view.delete() is called - when the context is deleted, the data tables that contain their expressions are cleaned up from the heap.

To implement this, I added a join function to t_data_table - given two data tables, it creates a new data table that references the columns from the source tables using their shared pointers without copying any of the underlying memory or allocating new data stores on the heap. When the context receives data from the gnode during notify, it is provided a t_data_table that contains the "real" columns joined with the expression columns from the context being notified. This means that the context will continue to treat expression columns as "real" columns so there is no loss of functionality.

Some work remains on the UI to allow for duplicate expression aliases to be created - right now, perspective-viewer does not allow aliases to be reused. Because #1426 changes the expressions UI "bigly", this PR does not contain any UI changes, and those changes will be added on in a new PR before the Perspective 0.9.0 release.

Changelog

Store expressions per-context using the t_expression_tables struct
t_vocab for string expressions are now per-context, instead of a static global vocab for all expressions
Added tests using multiple views with identical expression aliases
Added tests to assert that there are no memory leaks or excess allocations when views with expressions are created and deleted
Updated the View documentation to reflect the expressions API

WIP WIP working WIP WIP WIP

working for realsies properly construct vocab

texodus

Looks good! Check my review notes for a some debug cruft leftover.

There is a mostly-trivial performance regression here of about 5%, which is IMO acceptable for the massive improvement in code organization, test coverage, inline comments and general good engineering. We discussed t_data_table construction costs at length offline, and I do not suspect these are the cause of this slowdown - but IMO we should measure the impact of this anyway and more generally make some project-wide rules about shared_ptr vs. const ref vs. std::move just so we're consistent (as this is completely ad-hoc right now :) ).

texodus · 2021-05-29T01:01:34Z

cpp/perspective/CMakeLists.txt

@@ -250,6 +250,7 @@ if (PSP_WASM_BUILD)
 			-s DISABLE_EXCEPTION_CATCHING=0 \
 			-s ASSERTIONS=2 \
 			-s DEMANGLE_SUPPORT=1 \
+			-s SAFE_HEAP=1 \


IIRC this setting basically makes debug-build Perspective unusable (though it is quite useful for finding memory leaks!)

texodus · 2021-05-29T01:04:47Z

cpp/perspective/src/cpp/computed_expression.cpp

+    computed_function::order order_fn = computed_function::order(vocab);       \
+    computed_function::upper upper_fn = computed_function::upper(vocab);       \
+    computed_function::lower lower_fn = computed_function::lower(vocab);       \
+    computed_function::length length_fn = computed_function::length(vocab);    \


Any reason this needs to be a macro and not a function? The scope-capture of vocab specifically is something I'd like to stylistically avoid if possible. (Looks also like recompute is deprecated?)

I don't think this macro additionally captures the vocab - it is basically just inlining this code into where the macro is invoked, and the macro doesn't take vocab as a parameter. AFAIK since it's just a text replacement macro, it shouldn't capture vocab or have any notion of what vocab is during the preprocessor step. I think leaving it as a macro works well for readability in the compute function.

That's what macro variable capture means. There's little risk of vocab being mis-used here as you mention the macro won't compile, but for complexity analysis purposes I think its best if we avoid variable capture (in this case with a macro argument) and try where possible to force scope (with {}, though this doesn't work in this case since it introduces symbols) in our macros.

Regarding readability - this is identical to a function call, no?

texodus · 2021-05-29T01:06:38Z

cpp/perspective/src/cpp/context_grouped_pkey.cpp

@@ -42,9 +42,25 @@ t_ctx_grouped_pkey::init() {
    m_tree = std::make_shared<t_stree>(pivots, m_config.get_aggregates(), m_schema, m_config);
    m_tree->init();
    m_traversal = std::shared_ptr<t_traversal>(new t_traversal(m_tree));
+


Unrelated to this PR but - we should not compile this as there is no public API (nor tests ...)

texodus · 2021-05-29T01:10:43Z

cpp/perspective/src/cpp/context_zero.cpp

+    const std::string& colname,
+    const std::vector<t_tscalar>& pkeys,
+    std::vector<t_tscalar>& out_data) const {
+    std::shared_ptr<t_data_table> master_table = m_gstate->get_table();


This can go within the else block

texodus · 2021-05-29T01:12:06Z

cpp/perspective/src/cpp/data_table.cpp

+
+    for (const std::string& column : other_schema.m_columns) {
+        // Only append unique columns
+        if (!schema.has_column(column)) {


The alternative to this seems an error ...

texodus · 2021-05-29T01:27:31Z

packages/perspective/src/js/perspective.js

+            // Check if this expression is already in the array, if so then
+            // we need to replace the expression so the last expression tagged
+            // with the alias is the one that is applied to the engine.
+            if (expression_idx_map[expression_alias] !== undefined) {


This is impossible without a malformed config, no?

This is possible in the current UI, if one creates an expression column and then replaces it with another one - the viewer API will pass both expressions sequentially: ["// a \n 123", "// a\n upper('abcde')"]. With all the expression UI/fix PRs merged we can fix this on the viewer side.

texodus · 2021-05-29T01:28:39Z

python/perspective/perspective/table/_utils.py

@@ -160,22 +159,15 @@ def _parse_expression_strings(expressions):

        alias_match = re.match(ALIAS_REGEX, expression)


Note to self: ALIAS_REGEX is wrong (corrected in JS but not python ..)

texodus · 2021-05-29T01:31:13Z

packages/perspective/test/js/leaks.spec.js

+                const expression_schema = await view.expression_schema();
+                expect(Object.keys(expression_schema).length).toEqual(1);
+                await view.delete();
+            }, 3000);


These looks correct on-the-face, but why 3,000? In theory, these test should run identically with 1 or 1,000,000 - iteration checks are just there for offensively slow operations.

scripts/test_python.js

texodus · 2021-05-29T01:40:08Z

cpp/perspective/src/include/perspective/gnode.h

+        auto joined_delta = delta->join(ctx_expression_tables->m_delta);
+        auto joined_prev = prev->join(ctx_expression_tables->m_prev);
+        auto joined_current = current->join(ctx_expression_tables->m_current);
+        auto joined_transitions = transitions->join(ctx_expression_tables->m_transitions);


Can do this join once per context? The column sets do not change per gnode.process() call.

finos-cla-bot bot added the cla-present label May 26, 2021

sc1f marked this pull request as draft May 26, 2021 10:12

sc1f added 2 commits May 26, 2021 19:45

store expressions per-context using intermediate data tables

a367dab

WIP WIP working WIP WIP WIP

add tests, refactor use of vocab for expressions

478eaef

working for realsies properly construct vocab

sc1f force-pushed the improved-expressions branch from 725d256 to 478eaef Compare May 26, 2021 11:46

docs

0c591f3

sc1f marked this pull request as ready for review May 26, 2021 12:17

sc1f requested a review from texodus May 26, 2021 12:57

sc1f added C++ enhancement Feature requests or improvements JS Python labels May 26, 2021

sc1f added this to the 0.9.0 milestone May 26, 2021

texodus approved these changes May 29, 2021

View reviewed changes

texodus merged commit d721184 into master May 29, 2021

texodus deleted the improved-expressions branch May 29, 2021 01:41

sc1f mentioned this pull request Jun 4, 2021

Add integer, float, string, date, and datetime conversion functions #1437

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isolate expressions per-context and ensure memory stability #1431

Isolate expressions per-context and ensure memory stability #1431

sc1f commented May 26, 2021 •

edited

texodus left a comment

texodus May 29, 2021

texodus May 29, 2021

sc1f May 31, 2021

texodus May 31, 2021 •

edited

texodus May 29, 2021

texodus May 29, 2021

texodus May 29, 2021

texodus May 29, 2021

sc1f May 31, 2021

texodus May 29, 2021

texodus May 29, 2021

texodus May 29, 2021

		@@ -160,22 +159,15 @@ def _parse_expression_strings(expressions):

		alias_match = re.match(ALIAS_REGEX, expression)

Isolate expressions per-context and ensure memory stability #1431

Isolate expressions per-context and ensure memory stability #1431

Conversation

sc1f commented May 26, 2021 • edited

Changelog

texodus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texodus May 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sc1f commented May 26, 2021 •

edited

texodus May 31, 2021 •

edited