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 compatibility #70

Merged
merged 7 commits into from
Dec 8, 2023
Merged

Lua 5.1 compatibility #70

merged 7 commits into from
Dec 8, 2023

Conversation

Omikhleia
Copy link
Owner

@Omikhleia Omikhleia commented Dec 8, 2023

Closes #71

Stage 1:

  • Compile resilient manual (covers a bit good of all other dependencies incl. Djot, Markdown)
  • Compile books from awesome-sile-books
  • Compile markdown.sile manual --> breaks but due to things fixed here in the resilient.styles package.

Stage 2:

  • Apply same "min" Luacheck CI fix to all dependencies, fix issues and bump releases if need be:
    • barcodes.sile
    • couyards.sile
    • embedders.sile
    • fancytoc.sile
    • labelrefs.sile
    • markdown.sile -> update to 1.5.1
    • printoptions.sile -> Since we are heading toward a release, consider finalizing the minor PR there... (postponed)
    • ptable.sile -> update to 2.0.2
    • qrcode.sile
    • silex.sile
    • smartquotes.sile
    • spreadsheet.sile
    • teidict.sile
    • textsubsuper.sile -> update to 1.1.1

Stage 3 (release smoke tests)

  • Prepare releasing resilient.sile with updated dependencies
  • Compile resilient manual
  • Compile books from awesome-sile-books
    • LSG
    • lovecraft
  • Compile markdown.sile manual

@Omikhleia
Copy link
Owner Author

Omikhleia commented Dec 8, 2023

Luacheck CI update from Omikhleia/markdown.sile#101 yields:

    packages/resilient/bookmatters/init.lua:80:94: accessing undefined field pow of global math
    packages/resilient/poetry/init.lua:181:40: accessing undefined field log10 of global math

Yet they seem to be ok:

SILE v0.14.13 (LuaJIT 2.1.1700008891)
> math.pow(2,3)
8
> math.log10(10)
1

@Omikhleia
Copy link
Owner Author

Not seeing any noticeable performance improvement, so this SILE v0.14.13 (LuaJIT 2.1.1700008891) (from the initial docker file in sile-typesetter/sile#1923) is weird.

With -q -e 'print(SILE.lua_version); os.exit()' it reports 5.1 -- perhaps just running that, actually.

@Omikhleia
Copy link
Owner Author

Luacheck CI update from Omikhleia/markdown.sile#101 yields:

    packages/resilient/bookmatters/init.lua:80:94: accessing undefined field pow of global math
    packages/resilient/poetry/init.lua:181:40: accessing undefined field log10 of global math

math.pow(x,y) is deprecated in Lua 5.3 (https://www.lua.org/manual/5.3/manual.html#8.2) --> x^y is apparently the way to go and works in 5.1 too.

math.log10(x) was deprecated in Lua 5.2 (https://www.lua.org/manual/5.2/manual.html#8.2), which says to use math.log(x,10). Not working in 5.1 though, as shown below, so I guess one has to go for math.log(x)/math.log(10)....

$ lua5.2
Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio
> print(math.log10(5))
0.69897000433602
> print(math.log(5,10))
0.69897000433602

$ lua5.1
Lua 5.1.5  Copyright (C) 1994-2012 Lua.org, PUC-Rio
> print(math.log10(5))
0.69897000433602
> print(math.log(5,10))
1.6094379124341
> -- Doh, ignoring the base in Lua 5.1 !!!!!!!!!!!!!
> print(math.log(5)/math.log(10))
0.69897000433602

This language is insane.

@Omikhleia Omikhleia added this to the 2.2.1 milestone Dec 8, 2023
See related commits in
Omikhleia/markdown.sile#101

Co-authored-by: Caleb Maclennan <caleb@alerque.com>
Not a fix as those exist in Lua 5.1 to 5.4 at the time of writing,
but were marked deprecated in 5.2 and 5.3...
@alerque
Copy link
Contributor

alerque commented Dec 8, 2023

Yes, math in Lua is kind of insane. You haven't even scratched the surface of weirdness when you get into 32 bit vs. 64 bit numbers, bit-shift operators, etc. There isn't a clear succession of VMs because multiple parties that don't agree have continued developing the language in several directions.

For log10() it has been deprecated but not removed in any VM I know of, so you can add a luacheck exception for it and just use it for now, or as you noted do your own division:

$ for LUA in lua5.4 lua5.1 luajit; do $LUA -e 'print(math.log10(5))'; done
0.69897000433602
0.69897000433602
0.69897000433602
$ for LUA in lua5.4 lua5.1 luajit; do $LUA -e 'print(math.log(5)/math.log(10))'; done
0.69897000433602
0.69897000433602
0.69897000433602

@alerque
Copy link
Contributor

alerque commented Dec 8, 2023

Not seeing any noticeable performance improvement,

Between Lua 5.4 and LuaJIT? I've benchmarked the difference hundreds of times over the last couple years and LuaJIT is always consistently way faster. And not just 5% or 10% faster but 200–250%.

On the system I happen to be on right now with SILE master (not develop), the difference in rendering the SILE manual is 37.5s for Lua 5.4 vs. 18.9s for LuaJIT (or 16.2s for the OpenResty fork of LuaJIT). That's a bit over twice as fast.

The startup time for both is about the same so the difference on a one page document won't be huge, but the longer documents get the more obvious the difference should be.

@alerque
Copy link
Contributor

alerque commented Dec 8, 2023

With -q -e 'print(SILE.lua_version); os.exit()' it reports 5.1 -- perhaps just running that, actually.

The SILE.lua_version variable is reporting the ABI version, and LuaJIT is ABI 5.1 (hence why it can load and use the Rocks installed by luarocks --lua-version 5.1). You can see SILE.lua_isjit for a boolean status.

@Omikhleia
Copy link
Owner Author

The SILE.lua_version variable is reporting the ABI version, and LuaJIT is ABI 5.1 (hence why it can load and use the Rocks installed by luarocks --lua-version 5.1). You can see SILE.lua_isjit for a boolean status.

Thanks.

resilient -q -e 'print(SILE.lua_version); print(SILE.lua_isjit); os.exit()'
5.1
true

I'll do some perf comparisons after a clean reboot.

@alerque
Copy link
Contributor

alerque commented Dec 8, 2023

SILE requires and provides the compat53 patches. That should allow math.log() to accept a base in SILE running under Lua 5.1 even though plain Lua 5.1 does not.

On a second thought, we don't want troubles later, so let's also
enforce our vendored content is checked.
@Omikhleia
Copy link
Owner Author

Omikhleia commented Dec 8, 2023

On a second thought, we'll want to avoid troubles later, so will enforce linting on vendored libraries, and ideally propose fixes upstream
= affecting markdown.sile (for djot.lua and lunamark) and resilient.sile (tinyyaml though the situation is less clear here, see provided README)

EDIT: Acceptable warnings ignored in tinyyaml + opportunity to include a bug fix.

@Omikhleia
Copy link
Owner Author

Naive black-box performances - Let's go biblical.

So let's take a big file, compiling the Segond Bible...
Same initial conditions = no .toc and .ref files, resilient style file and master simplified to ensure same fonts, only standard desktop running on the laptop, near to no network activity.
Both tests are performed in sequence and use docker images (no other docker running on the host).

Lua 5.4 (custom made image, built some time ago with my rocks already installed, and a few extra fonts we are not going to use anyway)

$ time silex -u inputters.silm perfo/lsg.silm 
SILE v0.14.11.r21-g3b0ea5a (Lua 5.4)
<perfo/USX/001GEN.usx> as xml
[1] [2] [3] [4] 
...
[1709] [1710] [1711] <perfo/USX/066REV.usx> as xml
[1712] [1713] [1714] [1715] [1716] [1717] [1718] [1719] [1720] [1721] [1722] [1723] [1724] [1725] [1726] [1727] [1728] [1729] [1730] [1731] [1732] [1733] [1734] [1735] 
! Warning: table of contents has changed, please rerun SILE to update it.

real	32m21,037s
user	0m0,177s
sys	0m0,135s

LuaJIT 2.1 (made as stated above)

time resilient -u inputters.silm perfo/lsg.silm 
SILE v0.14.13 (LuaJIT 2.1.1700008891)
<perfo/USX/001GEN.usx> as xml
[1] [2] [3] [4] 
...
[1726] [1727] [1728] [1729] [1730] [1731] [1732] [1733] [1734] [1735] 
! Warning: table of contents has changed, please rerun SILE to update it.

real	20m44,838s
user	0m0,160s
sys	0m0,111s

Of course this is not a general test -- but on these two runs on big XML files with collated notes and references,

  • SILE on Lua 5.4 processed 0.9 pages / second
  • SILE on LuaJIT 2.1 processed 1.4 pages / second

Indeed better (x 1.56 boost).

@Omikhleia
Copy link
Owner Author

At some point though (near page 1200), underfull/overfull message start to differ...

@Omikhleia
Copy link
Owner Author

At some point though (near page 1200), underfull/overfull message start to differ...

Actually started much earlier.

image

At some points, likely chapters (= bible books, on a new page) it gets in sync, and the sequences repeat.

image

Could be that I don't have the exact same version of Gentium Plus in both images.
Could be floating number differences...
Or something more subtle.

It's a problem for having reproducible output for a given book (and anything > 1pt starts getting embarrassing), but I am not going to care here for now: this is acceptable for a pre-alpha software.

@alerque
Copy link
Contributor

alerque commented Dec 8, 2023

It would be pretty easy to check whether the difference is the font, the SILE version (you are pitting v0.14.11 against v0.14.13 here) or the Lua version. I very much suspect the SILE version is the culprit, with a second guess being the font version difference. I have yet to run into anything at all that successfully runs without errors but gives different metrics between Lua versions. I don't know what math you're doing in libraries apart from what SILE does, but I'm pretty confident SILE's math isn't being thrown off.

I have a lot of tooling for comparing builds done with various different options like different Lua versions or SILE versions, but I don't have your input content or font version to compare with.

@Omikhleia
Copy link
Owner Author

Omikhleia commented Dec 8, 2023

Yep, the test is not perfect (a 0.11 + cherry-picked patches that make it close to 0.13 but not so, vs. a real 0.13, both built at different times so maybe no getting the exact same font and tooling, etc.) -- Most important I guess is the perf. boost, which I assume to be somehow "correct" and cool. The rest would need a better testing procedure, admittedly!

@Omikhleia
Copy link
Owner Author

Omikhleia commented Dec 8, 2023

Note for future investigation: the "end" sequence might be easier to extract than things that occurs in the middle of books:

image

Everything was again in sync until Revelations, so it could be interesting to process just these few books

EDIT: they were in sync since [1629] [1630] [1631] [1632] [1633] [1634] <perfo/USX/051COL.usx> as xml -- so over nearly 100p.

@Omikhleia Omikhleia merged commit de7c21e into main Dec 8, 2023
2 checks passed
@Omikhleia
Copy link
Owner Author

Merging so I can start fetching dev luarocks and do some more tests.

@Omikhleia Omikhleia mentioned this pull request Dec 8, 2023
@Omikhleia Omikhleia deleted the fix-lua51-compat branch December 28, 2023 19:52
@Omikhleia
Copy link
Owner Author

For the record:

  • Final testing ("stage 3" in description above) done with SILE v0.14.14 (LuaJIT 2.1.1702233742)
  • Confirming my 0.14.11+ on Lua 5.4 and my 0.14.14 on LuaJIT don't have the exact same version of Gentium: resp. 5.0 vs. 6.2 -- I won't investigate the above-mentioned pagination difference, therefore.

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.

Lua 5.1 support
2 participants