Skip to content

Conversation

@tul
Copy link
Contributor

@tul tul commented Apr 23, 2019

Fixes #197 .

Changes proposed in this pull request:

  • Memory savings:
    • Can optionally set the LState's callstack to grow / shrink automatically (up to a max size)
    • Can optionally set the LState's registry to grow automatically (up to a max size)

It's been a long time since I opened #197 , sorry for the delay. Please feel free to feedback on this PR or ask for changes.

There are two parts to this PR - first the registry can be set to auto grow as needed. Originally, the registry was implemented as a fixed size slice. This PR simply allows this slice to be reallocated if it runs out of space. A maximum upper size to be set in the Options struct, as well as a grow step size. By default the RegistryMaxSize will be 0, which disables the auto grow behaviour and makes the code behaviour exactly the same as previously. As there is no penalty in this case, other than an inlined if statement, I have not abstracted the registry growth functionality via an interface.

The second part of this PR is the more complicated, it allows the callstack to be automatically resized as needed. This has been implemented now via an interface with the LState being configured on construction to either use the previous fixed size callstack, or the new auto growing callstack from this PR. The Options struct dictates which one should be used.

Abstracting the callstack into an interface is good in that it allows us to switch between implementations depending on the requirements : the auto growing one will use minimal memory, but has a slight performance penalty, whereas the fixed one will always use "worst case" memory, but will have predicable and fast performance.

Abstracting the callstack to be behind an interface has meant disabling the inlining which was in place for manipulating the stack from within the LState. I have added benchmarks which just do stack manipulation and I did not think the performance hit was so bad, but it might be worth benchmaking the new code (in both the fixed and auto growing configurations) against some actual lua benchmarking scripts, to see how it fares in actual usage.

@coveralls
Copy link

coveralls commented Apr 23, 2019

Coverage Status

Coverage decreased (-1.2%) to 88.423% when pulling 871bec7 on tul:auto_growing_stack_and_registry into 8bfc767 on yuin:master.

@yuin
Copy link
Owner

yuin commented Apr 24, 2019

Thanks for your great contribution! I'll check your PR this weekend.

@yuin
Copy link
Owner

yuin commented Apr 29, 2019

@tul LGTM! Could you update the README?

@tul
Copy link
Contributor Author

tul commented Apr 30, 2019

Yes, will do.

@tul
Copy link
Contributor Author

tul commented May 10, 2019

@yuin I've updated the README, please let me know if you would like anything clarifying or any further changes.

@yuin yuin merged commit 1a8ee40 into yuin:master May 13, 2019
@yuin
Copy link
Owner

yuin commented May 13, 2019

@tul LGTM, I've merged your PR. I appreciate your work!

@tul
Copy link
Contributor Author

tul commented May 13, 2019

Thanks - and thanks for all your work on gopher lua!

@tul tul deleted the auto_growing_stack_and_registry branch December 20, 2019 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory optimisations for discussion

3 participants