[[ VarAccess ]] Cleanup and optimization #5100

runrevmark · Jan 23, 2017

This is work-in-progress set of st-git patches which cleans up the implementation of variable access in the engine.

At present it includes the following:

Basic (incomplete) tests and benchmarks
Removal of some unused virtual methods from MCExpression (and descendants)
Changes uses of MCContainer to be mostly allocated 'on stack'
Moves all code to use just 'evalcontainer' which removes the 'evalvar' virtual method.
Unifies various hashing code from different places
Refactors code in MCName to do lookup and creation into a template based approach
Adds optimized code paths for creating 'index' names (integers which fit into index_t)
Ensures index name code paths are used when looking up array indices.
Adds the idea of purity to MCExpression (a pure MCExpression is one which has no side effects)
Uses templates to allow more efficient code paths for common MCVarref operations
Implements 'pure' code paths for set(into), give_value(into), dofree() and eval_ctxt MCVarref operations

This patch provides a substantial speed up to variable manipulation operations. Indeed, indexing operations using strings or literals (names) are 2-3 times faster (both storing and fetching); whilst indexing operations using integers are 4 times faster (storing) and 7 times faster (fetching). (All timings compared against 9dp4).

Note: This additionally changes the external interfaces so that internally they act on a container, rather than a variable - this should mean that externals should now be able to correctly set variables which are references to array elements... Tests need to be added for the externals interfaces to ensure that nothing has broken!

Note: The 'before' and 'after' operations still need optimized codepaths to be implemented; this should be combined with a refactoring of the code which manipulates Data and String values in this way since it is currently spread out in several places and the semantics are not 100% clear.

Note: There remains a fault with case-sensitivity being remembered for reference parameters - they are currently evaluated with the caseSensitive of the callee, rather then when the path was evaluated in the caller.

Note: The 'pure' dofree() code path is currently broken (I thought it might be because it is creating nested arrays when not necessary, but changing that does not seem to fix things - the IDE won't run properly with that code path enabled).

Note: For some reason one of the matchText tests fails without ensuring 'clearuql()' is done in MCVariable::setvalueref. There must be a subtle change somewhere which means the 'v0' undeclared var in the test is not getting cleared whereas it did before.

peter-b

This is awesome. 🥇

While you're at it, why not convert MCContainer::m_path to an MCAutoNameRefArray or something?

peter-b · Jan 23, 2017

engine/src/cmdse.cpp

-        MCContainer *t_container;
-        if (tptr -> evalcontainer(ctxt, t_container))
+        MCContainer *t_container = new(nothrow) MCContainer;
+        if (tptr -> evalcontainer(ctxt, *t_container))


At the moment, this will leak the container if evalcontainer() fails. I suggest an MCAutoPointer<MCContainer> here.

peter-b · Jan 23, 2017

engine/src/exec-keywords.cpp

-        MCContainer *t_container;
-        if (tptr -> evalcontainer(ctxt, t_container))
+        MCContainer *t_container = new(nothrow) MCContainer;
+        if (tptr -> evalcontainer(ctxt, *t_container))


This will also leak the allocated MCContainer instance.

peter-b · Jan 23, 2017

engine/src/externalv1.cpp

-		}
+    {
+        MCContainer *t_container;
+        t_container = p_parameters -> eval_argument_container();


Please use a managed lifetime here if possible!

I'm not sure it is possible as the MCParameter owns the MCContainer, and the MCReferenceExternalVariable will share it temporarily.

peter-b · Jan 23, 2017

engine/src/externalv0.cpp

+        return NULL;
+    }
+
+    MCArrayRef t_array = (MCArrayRef)*t_value;


This means that there are two pointers to the value live on the stack at the same time, one managed and one unmanaged. I suggest something like:

MCAutoArrayRef t_array = static_cast<MCArrayRef>(t_value.Take());

Using Take() avoids frobbing the reference count.

Also, don't forget that we're supposed to be avoiding C-style casts these days! ;-) There are a couple of examples like this.

Hmmm - I'm not sure this works with the current definition of the MCArrayRef constructor in MCAutoValueRefBase as it retains the incoming argument. Here we don't want to fettle with reference counts at all.

Good point. I suppose the alternative would be:

MCAutoArrayRef t_array = static_cast<MCArrayRef>(*t_value);

Admittedly this does require taking another reference to the value, but that's not necessarily a bad thing.

Well, it is a bad thing as the operation is atomic so the reference count bump is entirely unnecessary. I'd rather we 'fixed' the auto classes to allow movement of a MCValueRef into another auto container or a specific MCRef container - I'm just not sure what the 'correct' pattern for that is.

I'd rather we 'fixed' the auto classes to allow movement of a MCValueRef into another auto container or a specific MCRef container

Good idea. I'll think of something. 👍

peter-b · Jan 23, 2017

engine/src/variable.cpp

+        r_out[i + p_base.size()] = p_extra[i];
+
+    return true;
+}


Consider:

static MCAutoPointer<MCNameRef[]> __join_paths(MCSpan<MCNameRef> p_base, MCSpan<MCNameRef> p_extra) { MCAutoPointer t_result = new (nothrow) MCNameRef[p_base.size() + p_extra.size()]; if (t_result) { /* Populate t_result */ } return std::move(t_result); }

Then, when you use it, you can write:

MCAutoPointer<MCNameRef[]> t_full_path = __join_paths(MCMakeSpan(m_path, m_length), MCMakeSpan(p_path, p_path_length)); if (!t_full_path) return false;

This ensures RAII everywhere, and it's the pattern recommended by the C++ Guideline Standard for functions that create an owned value.

BTW I generally recommend MCMakeSpan() for creating spans in argument contexts like this.

This patch moves all implementations of hash functions to inline implementations in a private header foundation-hash.h. Hash implementations are provided for integer-like things, byte-like things and Unicode codepoint type things.

peter-b · Jan 23, 2017

@runrevmark Did you intentionally include the hash functions refactor in this PR?

runrevmark · Jan 23, 2017

@peter-b: Yes - it is an st-git stack so I can separate things out subsequently if necessary. The hash cleanup is required for an optimization to looking up elements in sequences.

This patch adds some tests and some benchmarks covering variable access operations.

This patch removes the 'rootmatches()' method from MCVarref along with its uses in the arithmetic commands. The latter used it to compute overlap between base and value expressions but this information is no longer needed.

This patch changes the creation of MCContainers so that the constructors act on a reference, rather than returning a new object. This allows stack allocation of MCContainer classes in almost all cases.

This patch removes the 'evalvar()' virtual method from MCExpression replacing all uses with 'evalcontainer()'.

The MCNameLookup method has been renamed to MCNameLookupCaseless to more accurately describe its function.

This patch adds a MCStringIsEqualToChars function to libfoundation allowing direct comparison between a MCStringRef and buffer of unichar_t.

This patch improves the performance of names when used as indexes into arrays. It adds an optimized codepath for looking up and creating names which are integers (in the range supported by index_t) and ensures these codepaths are used when converting from a value to a nameref if the initial value is a number.

This patch adds a virtual method 'is_pure' to the MCExpression class. This method returns true if evaluating the expression has no side-effects. Note: This is currently incomplete - only MCLiteral, MCConstant and MCVarref have a non-default implementation.

This patch adds an MCArrayMutateValue function to libfoundation. The function allows direct access to the slot of an element within an array, allowing the caller to assign to it directly, rather than having to fetch and then store.

This patch adds a 'capacity' field to MCAutoArrayRefBase allowing the auto class to either be used as a fixed size array, or a dynamically sizable array more easily.

This patch rewrites the resolve method to use a template class which is customizable. Customizations have been implemented for: - deferring evaluation of array keys (i.e. storing the full list of MCNameRefs) - fetching an array element when the dimensions are all pure (eval_ctxt) - storing into an array element when the dimensions are all pure (set and give_value) - removing an array element when the dimensions are all pure (dofree) Note: 'before' and 'after' mutation operations do not currently have a 'pure' code path.

This removes the pure path for dofree() and ensures a var being set via setvalueref is clearuql'd first (otherwise the matchText tests fail).

peter-b · Jan 24, 2017

engine/src/variable.cpp

+            t_result[i + p_base.size()] = p_extra[i];
+    }
+
+    return std::move(t_result);


I gave you duff information here. return implicitly moves its argument, so this can just be:

return t_result;

peter-b · Feb 3, 2017

engine/src/variable.cpp

+	r_container.m_variable = p_var;
+	r_container.m_length = 0;
+    r_container.m_path = nil;
+	r_container.m_case_sensitive = false;


This could also:

createwithpath(p_var, nullptr, 0, r_container;

Variable access performance improvements Cherry-picked from #5100.

runrevmark added enhancement LCS WIP labels Jan 23, 2017

peter-b reviewed Jan 23, 2017

View reviewed changes

[[ Foundation ]] Unify hash function implementations

f96547f

This patch moves all implementations of hash functions to inline implementations in a private header foundation-hash.h. Hash implementations are provided for integer-like things, byte-like things and Unicode codepoint type things.

runrevmark force-pushed the cleanup-var_related branch from b9db42f to 80f53c7 Compare January 23, 2017 10:37

runrevmark added 7 commits January 23, 2017 13:33

[[ VarAccess ]] Add tests and benchmarks

b2bebd2

This patch adds some tests and some benchmarks covering variable access operations.

[[ VarAccess ]] Remove 'rootmatches()' and uses from MCVarref

938780f

This patch removes the 'rootmatches()' method from MCVarref along with its uses in the arithmetic commands. The latter used it to compute overlap between base and value expressions but this information is no longer needed.

[[ VarAccess ]] Make MCContainer creation act on a reference

b645e79

This patch changes the creation of MCContainers so that the constructors act on a reference, rather than returning a new object. This allows stack allocation of MCContainer classes in almost all cases.

[[ VarAccess ]] Remove 'evalvar()' method

b0a6dfc

This patch removes the 'evalvar()' virtual method from MCExpression replacing all uses with 'evalcontainer()'.

[[ Foundation ]] Renamed MCNameLookup method

849f3a9

The MCNameLookup method has been renamed to MCNameLookupCaseless to more accurately describe its function.

[[ Foundation ]] Add MCStringIsEqualToChars

3074e0d

This patch adds a MCStringIsEqualToChars function to libfoundation allowing direct comparison between a MCStringRef and buffer of unichar_t.

runrevmark force-pushed the cleanup-var_related branch from 80f53c7 to e8c6ab2 Compare January 23, 2017 13:49

runrevmark added 5 commits January 23, 2017 14:12

[[ Foundation ]] Add MCArrayMutateValue function

780f477

This patch adds an MCArrayMutateValue function to libfoundation. The function allows direct access to the slot of an element within an array, allowing the caller to assign to it directly, rather than having to fetch and then store.

[[ Foundation ]] Add capacity field to MCAutoArrayRefBase

61c11e2

This patch adds a 'capacity' field to MCAutoArrayRefBase allowing the auto class to either be used as a fixed size array, or a dynamically sizable array more easily.

[[ WIP ]] Temporary fixes

01a2c1b

This removes the pure path for dofree() and ensures a var being set via setvalueref is clearuql'd first (otherwise the matchText tests fail).

peter-b reviewed Jan 24, 2017

View reviewed changes

peter-b reviewed Feb 3, 2017

View reviewed changes

peter-b mentioned this pull request Mar 9, 2017

Variable access performance improvements #5260

Merged

livecode-vulcan added a commit that referenced this pull request Mar 10, 2017

Auto-merge pull request #5260 from peter-b/var-access-improvements

b5b1627

Variable access performance improvements Cherry-picked from #5100.

peter-b mentioned this pull request Mar 14, 2017

libfoundation: Unify & improve fast hash functions #5278

Merged

runrevmark mentioned this pull request Apr 3, 2017

[[ Engine ]] Add the notion of an index name #5336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[[ VarAccess ]] Cleanup and optimization #5100

[[ VarAccess ]] Cleanup and optimization #5100

Uh oh!

runrevmark commented Jan 23, 2017 •

edited

Loading

Uh oh!

peter-b left a comment

Uh oh!

peter-b Jan 23, 2017

Uh oh!

peter-b Jan 23, 2017

Uh oh!

peter-b Jan 23, 2017

Uh oh!

runrevmark Jan 23, 2017

Uh oh!

peter-b Jan 23, 2017

Uh oh!

runrevmark Jan 23, 2017

Uh oh!

peter-b Jan 23, 2017

Uh oh!

runrevmark Jan 23, 2017

Uh oh!

peter-b Jan 23, 2017

Uh oh!

peter-b Jan 23, 2017

Uh oh!

peter-b commented Jan 23, 2017

Uh oh!

runrevmark commented Jan 23, 2017

Uh oh!

peter-b Jan 24, 2017

Uh oh!

peter-b Feb 3, 2017

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

[[ VarAccess ]] Cleanup and optimization #5100

Are you sure you want to change the base?

[[ VarAccess ]] Cleanup and optimization #5100

Uh oh!

Conversation

runrevmark commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peter-b left a comment

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!

peter-b commented Jan 23, 2017

Uh oh!

runrevmark commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

runrevmark commented Jan 23, 2017 •

edited

Loading