Review of PR#162 (module support)

Discuss the future of the AutoHotkey language
Post Reply
lexikos
Posts: 9635
Joined: 30 Sep 2013, 04:07
Contact:
Contact lexikos

Review of PR#162 (module support)

27 Dec 2022, 04:03

For those who aren't aware, @Helgef made a couple of attempts at implementing module support back in 2019 (pull request #142) and 2020 (pull request #162). Mostly behaviour, syntax and semantics were discussed. This post mostly isn't about that, but about the code.

As I've mentioned before, some time back I started reviewing PR#162 and didn't finish, instead diverting my attention to making major changes to the language and implementation. I had always intended to finish reviewing the code, figuring that I might gain some insights about implementing module support or improving the implementation in other ways.

So today I reviewed a diff of PR#162, which went a lot quicker than my previous attempt (reviewing each commit individually). I provide my thoughts below, for any interested reader. If I say "sm", I'm referring to the branch which contains the changes of PR#162.


Superseded

My initial review of sm inspired some changes to the syntax and implementation. sm was written mid-2020, and I'd guess that Helgef would do some things differently if he reproduced the module syntax today.

LineIsPrecededByParentLine() checks if the previous line is a parent line (like if/else/while/etc.), literally just mLastLine && ACT_IS_LINE_PARENT(mLastLine->mActionType). I would tend to avoid creating a helper function for something so simple, especially if defining and declaring the function signature is just as much code as the check itself. Because of changes in the way that blocks and lines are parsed, today I'm pretty sure this function could be replaced with a simple check of mPendingParentLine.


Line::mModule, g_CurrentModule

AutoHotkey already relies on a number of global variables during script execution. I would like to move away from this practice, not create more. For an example of a better design, consider Lua, which is written in C. Any functions that depend on or alter the script state take a parameter of type lua_State*. (But that's outside the scope of this PR or topic.)

mModule is copied to g_CurrentModule each and every time any line of code is executed, but I'd guess that g_CurrentModule isn't needed most of the time; only when a dynamic reference is being evaluated. This seems quite inefficient. Any code that needs the current module should be able to determine the current line, and from that, get the module.

As far as I can tell, a function cannot begin in one module and end in another module. Therefore, any line in a function necessarily belongs to the same module as the function. If mModule was a property of the function rather than each line, it would already be available through g->CurrentFunc->mModule. However, that won't work for code that isn't inside a function. One possible way to solve that would be to enclose module code in an anonymous function.

If we're introducing new complexity to the process of resolving names, it may be helpful to take into account future needs. For instance, lexically scoped variables are visible only within a given block, more specific than a function. Where before we had only global and function scope, in future we might have scope associated with a function block, a module block, or any other block.

So instead of associating a module with each line, we can associate each block-begin with a scope. Determining the scope within which to resolve a variable then becomes a task of locating the block-begin above the current line. This is a fairly trivial task, because each Line has a pointer to its mParentLine. Currently the block-begin belonging to a function has a pointer to the function; this would be replaced with something more general identifying the scope or function.


AddVar, FindVar, etc.

In order to support modules, sm adds a parameter to these functions for which module to add or find the variable in. However, local variables are still dependent on g->CurrentFunc, where g is global but dependent on the pseudo-thread, and CurrentFunc is updated when entering/leaving a function. AddVar handles built-in variables in a way completely independent of the module system, but ideally they would come from the std module.

sm also adds an optional parameter for AddVar to add an existing Var, in which case some parts of the function aren't relevant. I think that this has been completely superseded by changes since sm; i.e. VarList::Insert() would be used instead.

Future needs such as lexically scoped variables would require further changes to AddVar/FindVar. As with blocks (above), I feel that these functions should be revised to support scopes/namespaces in a more general way before (or while) implementing either modules or lexically scoped variables.


Macros Declaring Variables

Code: Select all

// Warning: This macro declares a variable outside the for block
#define FOR_EACH_MODULE(mod) int Macro_Index = 0; for ( ScriptModule *mod; mod = g_script.mModuleSimpleList.GetItem(Macro_Index); ++Macro_Index)
I think the commonplace C++ solution would be to define an iterator and use a proper for loop. For instance, the object defines a begin() iterator which points to the start of the list, an end() iterator which points to the end + 1, and a ++ operator which moves the iterator to the next item. Then for (auto *mod : g_script.mModuleSimpleList) can be used to iterate it.

I don't use this pattern, because in the common case of iterating from 0 to count-1, for (int i = 0; i < count; ++i) tends to produce slightly smaller compiled code than for (int i = 0; i != count; ++i), which is how the iterator pattern ends up looking to the compiler.

In general, it's not possible to declare two different types of variable in the for-loop's initializer, hence Macro_Index being declared outside the loop. However, the macro could have defined an extra block so that it would be self-contained, and wouldn't need to provide a warning.

For this case, there's actually a more elegant way to confine both variables to the for block:

Code: Select all

for (int Macro_Index = 0; ScriptModule *mod = g_script.mModuleSimpleList.GetItem(Macro_Index); ++Macro_Index)

Misc

Unless I'm writing code for both v1 and v2 (since v1 is still built with Visual C++ 2010), I tend to prefer using default member initializers. In 2010, it is necessary to list an initializer for each member in the constructor. If you have multiple constructors, it may be necessary to list the same members in each constructor. This repetition is inelegant and error-prone. For example (this is a modified excerpt from sm):

Code: Select all

class ScriptModule {
	Var** mVar, ** mLazyVar;
	int mVarCount, mVarCountMax, mLazyVarCount;
	
	ScriptModule(...) :
		mVar(NULL), mLazyVar(NULL),
		mVarCount(0), mVarCountMax(0), mLazyVarCount(0)
	{}
}

Code: Select all

class ScriptModule {
	Var** mVar = nullptr, ** mLazyVar = nullptr;
	int mVarCount = 0, mVarCountMax = 0, mLazyVarCount = 0;
	
	ScriptModule(...)
	{}
}

TCHAR result_buf[MAX_NUMBER_LENGTH]; is used with ResultToken for short string return values. In this case it doesn't end up getting used, but if it did, it would be insufficient. The result buffer needs to be MAX_NUMBER_LENGTH + 1 (MAX_NUMBER_SIZE) characters.

// Note: It seems like the loop isn't needed, i.e., it is inconsequentail to check mOuterFunc->mOuterFunc.
This part of the code would need to be rewritten due to changes since sm, but anyway, the loop was in fact needed to cover cases like the following.

Code: Select all

global x
outerouter() {
    local x
    outer() {
        inner() {
            (x) ; should be outerouter's x, not global x
        }
    }
}


Cleanup

Modules are deleted in Script::TerminateApp(), by iterating over the module list and deleting each module, then finally deleting the list itself. However, ~SimpleList() already includes code for freeing each item (which is skipped at runtime in this case), and ~SimpleList<ScriptModule*>() is called from ~Script() on exit. Modules should be cleaned up that way.

Freeing individual memory allocations on exit means larger code and more CPU cycles used, without any direct benefit. This is because 1) the C runtime already frees the entire heap when it unloads, and 2) the OS reclaims all memory allocated by the process when it exits. When we're in TerminateApp(), we know the process is exiting, so there's no need to do this cleanup.

However, maybe the destructors do more than free memory, or maybe they will in future. In future, maybe the ~Script() destructor will be called at some time other than process exit, in which case having it clean up properly becomes more important and useful. (TerminateApp originally include some other cleanup code, which I have recently moved in order to better support the use of AutoHotkey as a dll.)


Use of Objects

Originally I used an Object to map unresolved class names to objects during load. The Object stored exactly what was needed in a way that could be easily enumerated, and very little new code was needed.

sm replaces this with an Array, pushing a sequence of values reinterpreted as __int64, and popping them off in reverse order. I would never have even thought of doing it this way, and don't think it would have even been easier than the alternatives. Putting the values into a statically-typed UnresolvedClass struct would be much cleaner/more maintainable.

sm adds a template class named SimpleList that at first glance might seem to be suitable for avoiding boilerplate realloc code, but it has some unnecessary features that complicate matters and would probably increase code size. I've generally avoided std::vector and other STL classes for similar reasons.

A quick fix is to define struct UnresolvedClass and use FlatVector<UnresolvedClass>. FlatVector defines the basic operations we need, although it was originally designed for the purpose of conserving memory usage in objects (the length and size are stored with the allocated data, so a string buffer or list of zero capacity takes up less space).

This implementation reduces compiled code size by about 400 bytes, and makes the code a bit more maintainable.
Diff vs 210ae36b


Use of Script Functions

Script functions have a considerable amount of overhead, in terms of both CPU time and code size (perhaps in part due to inlining by the compiler). Using script functions from within C++ is ugly. sm uses StrSplit and InStr internally. I can only imagine this was done because Helgef is familiar with these functions and not familiar with C/C++. Still, I had trouble understanding why anyone would choose to implement it this way.

In one case, StrSplit is used to parse a simple comma-delimited list of names for UseVar/UseFunc. There are similar examples to draw from, like variable declarations, function parameter lists, etc. that are parsed using relatively efficient C++ code. It is quite trivial to do this without resorting to calling script functions or dynamically allocating an array.

This reduced code size only a little, but is somewhat cleaner.
Diff vs 210ae36b

In another case, StrSplit and InStr are used together to parse a full class name (module.outer.inner). It calls StrSplit(aString, ".") first to get ["module", "outer", "inner"], iterates over this, and on each iteration it calls InStr(aString, ".",,, A_Index) to find the relevant component within aString, which it would already have if it just parsed the string directly.

There was already code for parsing this dot-delimited list, and in fact, that code wasn't removed but isn't fully utilized by sm, because it is only called when there are no dots. It would have been much better to update that code to resolve modules first, then a global variable, then nested classes.

This reduced code size by nearly 1KB.
Diff vs 210ae36b
Top
Helgef
Posts: 4709
Joined: 17 Jul 2016, 01:02
Contact:
Contact Helgef

Re: Review of PR#162 (module support)

04 Mar 2023, 05:13

Thanks for taking the time to give the feedback :).

Although my memory about the details of the implementation is hazy, to say the least, I can say that I had little or no concerns about code size or code efficiency. My intention was a first draft of working and usable code. If there had ever been an agreement about design, many things would change ofc. BIFs and objects and most other choices I made about the implementation were efficient for me getting the job done.
This implementation reduces compiled code size by about 400 bytes, and makes the code a bit more maintainable.
I recall thinking, at the time, that either I implement this or no one else does (in the foreseeable future). That is, I'd rather have a useful version of modules today, than a useful version of modules which takes a few kb of space less x years in the future. Maintainability, I agree, it is a good thing to prioritise, but I think it conflicts with some of you ideas about code size. Just let the compiler handle such optimisations, by telling it. Why is the compiler setup for speed optimisation and not size if you want size optimisations? (you can have both by proper use of #pragmas). It is a futile approach anyways. Whatever produces smaller code today, might not in the future. Imo, the reason ahk exe isn't 1gb isn't because of concerns about byte-level optimisation, but the choice to not include 300 third-party dependencies.

Cheers.
Top
lexikos
Posts: 9635
Joined: 30 Sep 2013, 04:07
Contact:
Contact lexikos

Re: Review of PR#162 (module support)

09 Mar 2023, 02:08

It wasn't about optimizing code size, but about a more proper implementation, which happens to produce smaller code. In this case, I measured the code size to validate my assumption that it would produce smaller code. If you know that one way to implement a particular pattern is more efficient than another with no particular cost to development time or other things, then you can write better code in future.
Top
Post Reply

Return to “AutoHotkey Development”

Who is online

Users browsing this forum: No registered users and 13 guests