Updated ROTables 1st tranch #2742

TerryE · 2019-04-29T17:03:57Z

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/*.

Details

This is a large change. I am going to need to rebaseline against dev at the point of merge, but I wanted to make this available for review comment before this. The scope of the changes call into the following broad categories:

Changes to the rotable.h macros to support the new declarative style. These are largely in modules.h and rotable.h. Note that the eLua patch used two separate headers, rotable.hand rodefs.h. This second file has been dropped and the residual defines moved into rotable.h.
Retiring dead eLua options and code. For at least the last 4 years we have had to build the firmware (and later luac.cross) with a couple of hardwired eLua optimisation settings, and the Lua VM includes dead code that we can't use, so I've removed these options and stripped out this dead code.
Minor optimisations to the rotable lookup code. Since we only support character keys, there is no point in having a rotable key type as this doubles the number of flash work accesses in a ROTable lookup scan. Since the new declarative style hides these implementation details we do this doubling largely "for free".
Change to the Lua RTS to support the new macros. The base Lua libraries such as string (and os in the case of luac.cross) ave been updated to use the new macros.
Bulk largely automated edits of all of the module C files to use the new format ROTable declarations. In terms of files this was the bulk of the change, but this was largely automated though a bunch of ah-hoc sed scripts. Not that none of the functionality has been changed, though lookup is faster due to the optimisations discussed above.

…if OS not defined

…to Linux

jmattsson

Overall this is looking good. Some cleanup needed, but off to a good start here!

app/lua/lbaselib.c

jmattsson · 2019-05-01T01:41:59Z

app/lua/lbaselib.c

-  lua_pushrotable(L, (void *)BASE_ROTABLE);
-  lua_setglobal(L, "__index");
-#endif
+  luaL_register_light(L, "_G", &((luaL_Reg) {0}));


Wait what? Am I reading this right that we're registering a pointer to a temporary variable here? Shouldn't this be referring to base_funcs?

luaL_register_light() does multiple things. It registers the library in the global table (_G itself in this case) and also registers a list of functions. There aren't any functions to be registered here since these are in a ROTable for the baselib, so we either call luaL_register_light() with an empty list or clone and copy the subset of the functionality that we used inline. Doing the call is simpler than cut and paste.

jmattsson · 2019-05-01T01:45:19Z

app/lua/lrodefs.h

+#define LROT_FUNCENTRY(n,f)  {LRO_STRKEY(#n), LRO_FUNCVAL(f)},
+#define LROT_NUMENTRY(n,x)   {LRO_STRKEY(#n), LRO_NUMVAL(x)},
+#define LROT_LUDENTRY(n,x)   {LRO_STRKEY(#n), LRO_LUDATA((void *) x)},
+#define LROT_END(t,mt, f)    {LRO_NILKEY, LRO_NILVAL} };


Why the args on LROT_END, when none are being used?

Future proofing. A ROTable is currently a luaR_entry vector, so the Table code need to include conditional logic (is the address in Flash) ? do a ROTable reference : do a Table reference On the next version the LROT_END() will generate a luaR_entry terminator followed by a readonly Table rec which points to the vector. Generating the extra fields in the module source now means that we can implement this change without needing to change the modules again.

The main advantage of ROTables having a Table record is that we can push the RO vs RW abstraction down a level an dump a lot of conditional testing in the Lua VM where it only needs to reference and manipulate the table as an entity. The Table record contains useful stuff like the size of the table, and flags to indicate whether metas such as __index, _newindex and __gc exist.

The ROTable lookup cache now gives a better than 95% hit rate for key-found searches of ROTables. (I've tweaked the lookup algo to give more uniform coverage of the cache and to improve performance, BTW.) There is still a residual performance issue with key misses (even though this code runs about 4× faster than the eLua version). Having the abitity use the flabs byte to short-circuit frequent meta misses, and the length to allow you to sort the key list into ascending order and tag that you can do a binary search on the longer ROTables such as the baselib one, will pretty much remove this penalty all together.
So that in future we can re

jmattsson · 2019-05-01T01:46:22Z

app/lua/linit.c

+ * configuring builds with a small subset of the total modules.
+ *
+ * This linker-based approach is not practical for cross compiler builds which
+ * must link on a range of platforms, and were we down have control of PSECT


typo: "down" -> "don't"

Yup 😄 and also were -> where

jmattsson · 2019-05-01T01:49:53Z

app/lua/linit.c

-      lua_pushcfunction(L, lib->func);
-      lua_pushstring(L, lib->name);
-      lua_call(L, 1, 0);
+#ifndef LUA_CROSS_COMPILER


This looks like it could be pruned before merge.

Yup, but I use this test point so frequently for debugging startup that having left as a commented out hook is simpler. I would prefer to leave this in as it is only a comment for production use.

app/lua/lrotable.h

app/lua/lstrlib.c

jmattsson · 2019-05-01T02:03:00Z

app/lua/ltable.c

@@ -580,7 +580,7 @@ const TValue *luaH_getnum (Table *t, int key) {

 /* same thing for rotables */
 const TValue *luaH_getnum_ro (void *t, int key) {
-  const TValue *res = luaR_findentryN(t, key, NULL);
+  const TValue *res = NULL;  // integer values not supported: luaR_findentryN(t, key, NULL);


Do we get a compile-time error if someone does try to register a numeric key in an RO table? If not, can we?

There is no macro to do this, so they can't. None of our current code uses numerically indexed ROTables, and there is roughly a 2× performance hit in leaving in this feature that we don't use, and so I removed it.

If there we want to add it back we should think about how we could do this but we should have this as a discussion on the issue, as there are trade-offs here.

app/lua/ltable.c

jmattsson · 2019-05-01T13:28:13Z

Thanks for the explanations & clarifications. I am satisfied you know what you're doing here :D

Happy for you to merge once you've done the discussed tidying up.

marcelstoer · 2019-05-03T06:24:05Z

Not sure if you or Johny noticed but the CI build failed.

gpio.c:323:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'extern'
 extern int gpio_pulse_init(lua_State *);
 ^
In file included from ../include/module.h:5:0,
                 from gpio.c:4:
gpio.c:333:25: error: 'gpio_pulse_map' undeclared here (not in a function)
   LROT_TABENTRY( pulse, gpio_pulse )
                         ^
../lua/lrotable.h:16:39: note: in definition of macro 'LRO_ROVAL'
 #define LRO_ROVAL(v)    {{.p = (void*)v}, LUA_TROTABLE}
                                       ^
gpio.c:333:3: note: in expansion of macro 'LROT_TABENTRY'
   LROT_TABENTRY( pulse, gpio_pulse )
   ^
gpio.c: In function 'luaopen_gpio':
gpio.c:351:3: error: implicit declaration of function 'gpio_pulse_init' [-Werror=implicit-function-declaration]
   gpio_pulse_init(L);

TerryE · 2019-05-03T18:10:17Z

@marcelstoer, thanks Marcel, yes I did. There are a few edge cases with table references that barfed on the CI build. I'll clean these up tomorrow before the merge.

TerryE · 2019-05-07T22:00:02Z

If we don't have any more issues, then I'll merge this into dev tomorrow.

TerryE added 10 commits April 20, 2019 01:45

1st Tranch of SDK 3.0 follow up changes

a37ad42

2nd Tranch of SDK 3.0 follow up changes

b77d371

Updated ROTables 1st tranch

aa2e29d

fix dependency: sdk-pruned needs AR and hence toolkit

3a3b2df

2nd attempt at fixing make dependency

26110b0

Find out what the MAKE_HOST is

5829fb5

Use OS variablle to determine platform or (MAKE_HOST contains linux) …

fc480ac

…if OS not defined

Use OS then MAKE_HOST variablles to determine platform or defaulting …

6f3a93a

…to Linux

Add the get and set partition tables functions and document in node.md

a89dd18

remove some trailing spaces that have crept in during changes

6b08993

jmattsson requested changes May 1, 2019

View reviewed changes

jmattsson approved these changes May 1, 2019

View reviewed changes

Merge branch 'dev' into dev-ROTables-update

bf10af2

Rework following review

9ed4580

TerryE merged commit 1990f95 into nodemcu:dev May 8, 2019

TerryE mentioned this pull request May 10, 2019

new pwm2 module #2747

Merged

4 tasks

marcelstoer added this to the Next release milestone May 21, 2019

marcelstoer mentioned this pull request Jun 1, 2019

ADS1115 adc1:setting #2753

Closed

shimosaurus mentioned this pull request Jun 21, 2019

U8g2: display drivers are not compiled #2806

Closed

TerryE deleted the dev-ROTables-update branch August 21, 2020 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated ROTables 1st tranch #2742

Updated ROTables 1st tranch #2742

TerryE commented Apr 29, 2019 •

edited

Loading

jmattsson left a comment

jmattsson May 1, 2019

TerryE May 1, 2019

jmattsson May 1, 2019

TerryE May 1, 2019 •

edited

Loading

jmattsson May 1, 2019

TerryE May 1, 2019

jmattsson May 1, 2019

TerryE May 1, 2019

jmattsson May 1, 2019

TerryE May 1, 2019

jmattsson commented May 1, 2019

marcelstoer commented May 3, 2019

TerryE commented May 3, 2019

TerryE commented May 7, 2019

Updated ROTables 1st tranch #2742

Updated ROTables 1st tranch #2742

Conversation

TerryE commented Apr 29, 2019 • edited Loading

Details

jmattsson 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

TerryE May 1, 2019 • edited Loading

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

jmattsson commented May 1, 2019

marcelstoer commented May 3, 2019

TerryE commented May 3, 2019

TerryE commented May 7, 2019

TerryE commented Apr 29, 2019 •

edited

Loading

TerryE May 1, 2019 •

edited

Loading