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

new --strip option to build bundle #276

Merged
merged 5 commits into from
May 11, 2023
Merged

new --strip option to build bundle #276

merged 5 commits into from
May 11, 2023

Conversation

zhaozg
Copy link
Member

@zhaozg zhaozg commented May 10, 2023

mainly to hide Lua source code, and size and speed are byproduct.

@Bilal2453
Copy link
Contributor

Bilal2453 commented May 10, 2023

Awesome! I've been wanting to implement this for a while but I instead relied on an external script to do it for me before bundling.

The community on Discord have also had a conversation about this (multiple ones in fact), and a comparison between bundled: raw Lua code vs minified code vs the bytecode over here.

First, this was in fact already implemented, but over in Lit, then later was removed in luvit/lit#263 for having bigger overall bundles and introducing a compatibility hell when building other apps.

From my testing, this does in fact offer reduced size... a very big reduction. It only rarely resulted in slight overall size (with very small files), I tested it with multiple projects, mainly this project and inspect.lua, both resulted in a massive drop in size.

The test was done as follows:

  • compiled with luajit -b raw.lua ./luajit.out
  • compressed the luajit.out file with zip result.zip ./target

The results were as it follows:

raw: 29.1kB
bytecode + compression: 4.0kB
raw + compression: 8.2kB

It is also worth noting that the minified Lua code did result in a better overall size:

minified raw: 7.8kB
bytecode + compression: 4.0kB
minified raw + compression: 2.7kB

But that is impractical to implement and bundle, so it is out of the question.

Seems like the main reason Lit failed at this was it didn't strip debugging symbols out (not passing the second argument of string.dump), which indeed results in increased overall size.

Remains the compatibility issue, which should not be present here as that was Lit mismatching Luvi versions. It does mean the compiled code will only work on the Luvi version that built it, but that sounds OK as it is never ran anywhere else really.

Do note that in case Luvi was using Lua 5.1 or Lua 5.2 then the compiled bytecode will most likely be bigger in size than original, as those do not support the strip argument in string.dump.

Overall I am in favor of this addition! I will also redo my tests using loadstring/string.dump to verify the results.

@Bilal2453
Copy link
Contributor

Bilal2453 commented May 10, 2023

@zhaozg Seems like this should probably fallback to simply writing the un-compiled version of the string when it fails on load? For example Lit has a tests/bad-package.lua test which introduce bad syntax, ./luvi ./lit --strip -o lit fails but ./luvi ./lit -o lit works. The error:

Creating new binary: /home/bilal/Desktop/luvi-bundle-test/lit-normal
Copying initial 1612360 bytes from /home/bilal/Desktop/luvi-bundle-test/luvi-strip
Zipping /home/bilal/Desktop/luvi-bundle-test/lit
   ...
    tests/bad-package/package.lua
...lal/Desktop/luvi-strip-build/luvi/src/lua/luvibundle.lua:196: [string "tests/bad-package/package.lua"]:6: '}' expected (to close '{' at line 4) near '"creationix/[email protected]"'
stack traceback:
	[C]: in function 'assert'
	...lal/Desktop/luvi-strip-build/luvi/src/lua/luvibundle.lua:196: in function 'copyFolder'
	...lal/Desktop/luvi-strip-build/luvi/src/lua/luvibundle.lua:191: in function 'copyFolder'
	...lal/Desktop/luvi-strip-build/luvi/src/lua/luvibundle.lua:191: in function 'copyFolder'
	...lal/Desktop/luvi-strip-build/luvi/src/lua/luvibundle.lua:205: in function <...lal/Desktop/luvi-strip-build/luvi/src/lua/luvibundle.lua:154>

@Bilal2453
Copy link
Contributor

I've decided to test this for size by applying the patch and testing it directly, more representative like that. Also modified this a bit to solve the above issue and successfully bundle Lit, in luvibundle.lua:

          local ctx = bundle.readfile(child)
          if strip and child:sub(-4, -1)=='.lua' then
            local fn = load(ctx, child)
            if fn then
              ctx = string.dump(ctx, strip)
            end
          end
          writer:add(child, ctx, 9)
         end

The "Normal" type will use an unpatched version of Luvi ran using ./luv ./source -o output, the "Stripped" will use Luvi with this patch applied and ran with ./luvi ./source --strip -o output.

Bundling latest Luvit:

Type Size
Normal 2035214 bytes (2.035214MB)
Stripped 2006847 bytes (2.006847MB) (1.39% improvement)

Bundling latest Lit:

Type Size
Normal 1859290 bytes (1.85929MB)
Stripped 1844580 bytes (1.84458MB) (0.8% improvement)

Bundling inspect.lua (as a single main.lua):

Type Size
Normal 1611442 bytes (1.611442MB)
Stripped 1615839 bytes (1.615839MB) (0.27% worse)

(Should I test other things out?)

Seems like overall, there is a size improvement although it is somewhat negligible, in case of inspect.lua the size of the stripped version was slightly bigger.

I think this is fine, as long as it is not an increase in size (most often) then the rest of the use cases this will support are worth it.

@zhaozg
Copy link
Member Author

zhaozg commented May 11, 2023

Seems like this should probably fallback to simply writing the un-compiled version of the string when it fails on load? For example Lit has a tests/bad-package.lua test which introduce bad syntax,

Do you really want to bundle lua code with errors, I think we can give more solution.

  1. don't compile package.lua, which is metadata in luvi.
  2. add --force to continue with errors.
  3. add --copy keep as old behavior.

@zhaozg
Copy link
Member Author

zhaozg commented May 11, 2023

cc @luvit/luvit-admin

@squeek502
Copy link
Member

squeek502 commented May 11, 2023

It seems like the new default behavior without any options is to compile the Lua but not strip, but I think --copy should be the default, and that the --copy option should be removed.

Here's my preference for what the help should look like:

  --compile-lua    Compile Lua code into bytecode before bundling.
  --strip          Strip debug info from compiled Lua code.
  --force          Ignore errors when compiling Lua code.

src/lua/luvibundle.lua Outdated Show resolved Hide resolved
src/lua/luvibundle.lua Outdated Show resolved Hide resolved
@zhaozg
Copy link
Member Author

zhaozg commented May 11, 2023

It seems like the new default behavior without any options is to compile the Lua but not strip, but I think --copy should be the default, and that the --copy option should be removed.

Ok, I'll fix.

I prefer --compile more than --compile-lua

@zhaozg zhaozg closed this May 11, 2023
@zhaozg zhaozg reopened this May 11, 2023
@zhaozg zhaozg requested a review from squeek502 May 11, 2023 06:52
Copy link
Member

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

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

If this is going to be the case:

local compile = options.strip or options.compile

then I think these suggested changes make sense. Alternatively, we could make --strip do nothing unless --compile is set.

I don't have a preference for which we should do.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/lua/init.lua Outdated Show resolved Hide resolved
@zhaozg
Copy link
Member Author

zhaozg commented May 11, 2023

then I think these suggested changes make sense. Alternatively, we could make --strip do nothing unless --compile is set.

--strip flag to indicate strip compiled lua bytecode, let --compile default on。
short alias -s can be used more frequently, don't need type both --compile and --strip.

Is this easy to be ambiguous?

@zhaozg
Copy link
Member Author

zhaozg commented May 11, 2023

@squeek502 thanks your code review. A nice and meticulous person.

@zhaozg zhaozg merged commit 3970abc into luvit:master May 11, 2023
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.

3 participants