Disable lua compilation based on file size#263
Merged
squeek502 merged 1 commit intoluvit:masterfrom Jan 25, 2020
SinisterRectus:master
Merged
Disable lua compilation based on file size#263squeek502 merged 1 commit intoluvit:masterfrom SinisterRectus:master
squeek502 merged 1 commit intoluvit:masterfrom
SinisterRectus:master
Conversation
squeek502
approved these changes
Oct 8, 2019
Member
squeek502
left a comment
There was a problem hiding this comment.
Nice, definitely think this is a worthwhile change.
Member
|
Whoops, forgot to merge this. |
Member
Author
|
No worries. I wasn't sure that you had planned to anyway. |
squeek502
added a commit
to squeek502/lit
that referenced
this pull request
Mar 9, 2020
….exe If there was an older lit.exe or luvi.exe on your PATH, then running get-lit.ps1 anywhere would use those older existing lit.exe/luvi.exe instead of the newly downloaded ones. This could lead to 'cannot load incompatible bytecode' errors that should no longer be possible after luvit#263.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lit currently bundles compiled .lua files if their bytecode is smaller in size than the original source file. I propose that we change this.
Issue 1: The attempt at space saving is negligible or detrimental.
Using lit itself as an example of a bundled luvi 2.9.3 app on Windows, the resulting file sizes are:
No compilation: 4,123 KB
Partial compilation (current behavior): 4,144 KB
Full compilation: 4,148 KB
While there is sometimes a reduction in individual .lua file sizes, there is actually an overall increase in total executable size here. Presumably bytecode is being zipped with less efficiency than the source? Additionally, luvi contributes over 90% of the executable size here, so there isn't much room for space negotiation.
Issue 2: Lua compiled using one luvi version may be incompatible with other versions.
More specifically, if the version of luvi used to build lit is different from the version of luvi that lit uses to build another app, the bytecode may not be compatible if different luac versions are involved. See #252.
Precompiling to bytecode isn't inherently bad! We just aren't using it in the right way. From the luac man page:
What this PR does is remove the logic for compiling based on file size, makes compilation and all-or-nothing procedure, and changes the default behavior to no compilation.
What this PR does NOT do is add the option to select whether files should be compiled from the CLI. I'm open for suggestions on how to handle that, or we can save that for a later change.
Fixes #252.
Happy Hacktoberfest