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

cron memory management with Lua table #3159

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

cron memory management with Lua table #3159

wants to merge 3 commits into from

Conversation

nwf
Copy link
Member

@nwf nwf commented Jun 13, 2020

Refactor cron internals to let the Lua GC do more of the work -- that is, store the list of scheduled cron elements as a Lua table. While here, permit an entry's schedule() function to reuse the existing schedule.

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

@nwf nwf force-pushed the cron-tidy branch 2 times, most recently from 89ad020 to 12e3795 Compare June 13, 2020 05:08
@marcelstoer marcelstoer added this to the Next release milestone Jun 13, 2020
// Allocate userdata

// Grab the table of all entries onto the stack
lua_rawgeti(L, LUA_REGISTRYINDEX, cronent_table_ref);
Copy link
Collaborator

@TerryE TerryE Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cronent_table_refcontains a list of ud's and we scan for empty slots, scan for a specific one etc. I just wonder if we would we better off using a keyed table with the ud as the key, and just let the table access functions do all this for us.

The comment below isn't what the scan does, which is scan from the start of the table looking for the first null slot.

On a separate note, just in terms of efficiency scanning up a list for a null slot must terminate (possibly by throwing and EOM) so this scan could be more efficiently written:

  while(lua_rawgeti(L, -1, ix) != LUA_TNIL) { lua_pop(L, 1); ix++; }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoop, yes, the comment's stale.

I thought about using the ud as both the key and value, but this was a straightforward bring-over of existing logic, AIUI array-like tables are smaller in memory, and I think we should drop table entirely and just equip each cron entry with a timer as part of fixing #3160.

@marcelstoer
Copy link
Member

CI build failure

cron.c:95:3: error: void value not ignored as it ought to be
   while(lua_rawgeti(L, -1, ix) != LUA_TNIL) {

@nwf
Copy link
Member Author

nwf commented Jun 20, 2020

Ah, I think that's a 5.3 vs 5.1 thing. Let's hold on this PR until @TerryE's done the requisite backporting? Or I could go put the call to lua_type back.

@nwf nwf added the blocked Waiting on another PR to land label Jun 20, 2020
@nwf nwf removed this from the Next release milestone Sep 1, 2020
@nwf nwf added this to the Next release milestone Sep 29, 2020
@nwf nwf removed the blocked Waiting on another PR to land label Sep 29, 2020
@nwf nwf requested a review from TerryE October 6, 2020 10:27
@marcelstoer marcelstoer removed this from the Next release milestone Nov 3, 2020
Pin a table to the registry and use that instead of manually allocating
and reallocating and shuffling integers around.

Thanks to @TerryE for some style suggestions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants