Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua 5.1 to 5.3 Realignment Phase 3 and baseline 5.3 #3075

Merged
merged 2 commits into from
Apr 27, 2020

Conversation

TerryE
Copy link
Collaborator

@TerryE TerryE commented Apr 24, 2020

Replaces #2912 and thus addresses: #2808, #2839 (partial), #2858, #2890

To be addressed before merge: #2783

  • 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/*.

This is replacement for #2912. We've a lot of changes to dev since the original version that the previous PR was based on that I couldn't update the PR after the latest merge without loosing the commit history so it was just easier to reissue the PR.

  1. Tweaks to standard lua.h and lauxlib.h C API to provide a unified API subset for our app/modules.

  2. Updates to app/lua to realigned APIs. The Lua 5.3 version significantly improves ROTable performance and introduces of function and table subtypes. These have regressed into the app/lua codebase so that both the 5.1 and 5.3 variants support this unified API.

  3. Updates to app/modules and supporting directories to realign APIs. A general cleanup of the modules to use the stricter Lua API implemented in (2). Modules now only access the C API through the public lua.h and lauxlib.h interfaces.

  4. Addition on the app/lua53 code tree to implement the Lua 5.3 VM and runtime. This includes the core changes needed to be able to build LUA=53 and download a runnable Lua 5.3 firmware image onto the ESP.

  5. Common lua.c for both versions. lua.cis the initiation framework to start the Lua VM and runtime. This one module is highly specific to the interface with the non-OS SDK. This code is now shared between the Lua51 and Lua53 frameworks without use of symlinks. See app/lua/lua.c.

The whitepaper describing the scope of this work in detail is my gist: Lua 5.3 port to NodeMCU Firmware.

-  Lots of minor but nasty bugfixes to get all tests to run clean
-  core lua and test suite fixes to allow luac -F to run cleanly against test suite
-  next tranch to get LFS working
-  luac.cross -a options plus fixes from feedback
-  UART fixes and lua.c merge
-  commit of wip prior to rebaselining against current dev
-  more tweaks
@TerryE
Copy link
Collaborator Author

TerryE commented Apr 24, 2020

I want to validate that this merge was correct. If not then I will close and reissue.

@nwf
Copy link
Member

nwf commented Apr 24, 2020

I have hoisted my dev-active branch atop this merge and modulo a few changes in local projects, things seem OK (ETA: for lua5.1; I haven't tested with 5.3 yet).

It does appear that this branch regressed 49ac968, tho'; nodemcu-partition.py should be using args.fs not fs.args.

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 25, 2020

Other misc details / discussion points

  • One small change that I have done is to remove out-of-VM references to lua_input_line() so app/coap/endpoints.c:L180-187 now does the Lua source execution the standard Lua way and node.input() pushes the input to the stdin pipe.
  • A lot of our modules use the internal VM memory allocator (color_utils, cron, crypto, encoder, enduser_setup, file, gpio, gpio_pulse, hx711, net, node, somfy, tls, websocket, ws2812, ws2812_effects) instead of the malloc allocation. This was something that I promoted since the internal allocator works correctly with the Lua GC and can trigger GC prior to allocation if needed. However, strictly lmem.c is not part of the C API and should be reserved for core Lua code. The approved way of doing user resource allocation is to use Userdata and since I've got to understand the Lua VM better, I've come to realise that there no material disadvantages of doing this and a lot of GC advantages. Something that we need to revisit, I think.
  • The modules all compile clean but there are still odd things that need tyding up. For example the ROTable macros are really part of the public API and so should be in lua.h and ditto the couple of LFS routines, luaN_index() and luaN_reload_reboot(), that are called from node.c; I will rename these as lua_lfsindex() and lua_lfsreload() and move these and the LROT defines into lua.h
  • I also need to review the other external components such as spffsimg and nodemcu-partition.
  • At the moment there is no easy way to determine actual size of the LFS region used by a given luac.cross compile. This seems be unsatisfactory since you you will only discover if an LFS image is too big for a given region when you attempt to load it. Getting this actual size out of the compile seems a highly desirable addition.

But all of this can be refined in the light of serious testing feedback. What we really need now is for this to occur so that we can merge this branch into dev with confidence.

@tomsci
Copy link

tomsci commented Apr 25, 2020

The approved way of doing user resource allocation is to use Userdata [...]
no material disadvantages of doing this and a _lot_of GC advantages. Something that we need to revisit, I think

Absolutely, agreed! It makes for slightly more complex code but (perhaps counterintuitively) the code tends to be less likely to have memory leaks, as temporary raw unowned pointers become userdatas on the stack instead and thus will get cleaned up if the function returns early for eg.

I've been trying to avoid ever using the luaM_ functions and would definitely encourage others to do the same.

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 25, 2020

It makes for slightly more complex code but (perhaps counterintuitively) the code tends to be less likely to have memory leaks, as temporary raw unowned pointers become userdatas on the stack instead and thus will get cleaned up if the function returns early.

This feature actually makes the code simpler: as you say temporary resources are referenced on the Lua stack and get dereferenced if and when you leave the routine for any reason, and the LGC will collect this memory on the next sweep.

@nwf nwf removed this from the Next release milestone Apr 25, 2020
@TerryE
Copy link
Collaborator Author

TerryE commented Apr 25, 2020

@HHHartmann @nwf perhaps the alternative to moving the LROT macros into lua.h is to reserve lnodemcu.h for the NodeMCU-specfic public C API extensions.

@nwf
Copy link
Member

nwf commented Apr 25, 2020

I agree re: the use of userdata for C-side allocations whenever possible, but... a class of bug I have found in our C modules is that they intermixed allocations, initialization, and registrations with the SDK and derived subsystems. The result could be that, in OOM scenarios, live objects pointed to allocated but not fully constructed objects. For example, luaL_ref can abort if it cannot allocate memory to grow the registry table, and aborts show up as longjmp-ing back over the rest of the constructor.

I have taken the approach of squashing allocations up into the userdata -- rather than pointing to the espconn structure, for example, tls and mqtt now embed these in their userdata. Some deeper allocations are unavoidable, it seems, and so those need to be carefully sequenced with attention to scenarios where Lua might abort.

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 25, 2020

In general using userdata helps / simplifies things when the function aborts due to out of memory. The process should be (i) create the resources and leave them on the stack (ii) once all resources have been created then hook them where they belong. Temporary resources will be GCed after return. The issues arise if they need to be bound to other UDs as you can only do this with the Lua 5.3 API and so this is approach isn't portable. Where practical it is better to concatenate all dynamic resources into a single UD brick.

There will always be a couple of modules such as net and tls where this can all get hairy, but these will always be hairy; what I am really thinking about are the typical C modules that other contributors ad and maintain

@nwf nwf merged commit 9ef5c7d into nodemcu:dev Apr 27, 2020
@nwf
Copy link
Member

nwf commented Apr 27, 2020

@TerryE has indicated that he will fix issues identified with this PR by adding follow-up commits rather than revising these. As per #3066, there is desire to merge this and then ship a new master release. Therefore, I have merged to dev.

@TerryE
Copy link
Collaborator Author

TerryE commented Apr 27, 2020

👍 Will do

@marcelstoer
Copy link
Member

addresses: #2808, #2839 (partial), #2858, #2890

Can we, therefore, close #2808 and #2890?

@TerryE TerryE mentioned this pull request May 29, 2020
nwf added a commit to nwf/nodemcu-firmware that referenced this pull request May 31, 2020
Replay a line from nodemcu#2861
after accidental revert in
nodemcu#3075 (specifically
9ef5c7d)
TerryE pushed a commit that referenced this pull request Jun 1, 2020
Replay a line from #2861
after accidental revert in
#3075 (specifically
9ef5c7d)
marcelstoer pushed a commit that referenced this pull request Jun 9, 2020
-  Lots of minor but nasty bugfixes to get all tests to run clean
-  core lua and test suite fixes to allow luac -F to run cleanly against test suite
-  next tranch to get LFS working
-  luac.cross -a options plus fixes from feedback
-  UART fixes and lua.c merge
-  commit of wip prior to rebaselining against current dev
-  more tweaks
marcelstoer pushed a commit that referenced this pull request Jun 9, 2020
Replay a line from #2861
after accidental revert in
#3075 (specifically
9ef5c7d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants