libvips / lua-vips

Lua binding for the libvips image processing library
MIT License
125 stars 10 forks source link

Add bitops #69

Closed rolandlo closed 7 months ago

rolandlo commented 7 months ago

luaffi-tkl, which is used on Lua 5.x to replace the LuaJIT builtin ffi module, has dropped shipping its bit-module (which only worked on Lua >= 5.3). This PR replaces the bit-module via bitops.lua. Only the bitwise and on two arguments is needed.

For LuaJIT: use builtin bit.band For Lua 5.1, 5.2: use bit.band from luabitop if available For Lua 5.3, 5.4: use builtin & operator

On Lua 5.1, 5.2, which are newly supported, luabitop becomes a dependency.

RiskoZoSlovenska commented 7 months ago

Wouldn't it be simpler and easier to just specify luabitop as a LuaRocks dependency and use that on all versions (including LuaJIT)?

Personally, I'd like to try and get rid of the Makefile in the future; LuaRocks should support doing almost everything the Makefile does.

rolandlo commented 7 months ago

Wouldn't it be simpler and easier to just specify luabitop as a LuaRocks dependency and use that on all versions (including LuaJIT)?

luabitop requires lua >= 5.1, < 5.3. If we make luabitop as a dependency of lua-vips we can't install lua-vips for versions 5.3 and 5.4 any more, I fear. Or is there a way to specify dependencies only for particular Lua versions?

RiskoZoSlovenska commented 7 months ago

Oh, hmm, that's annoying. I'm not aware of any way to specify dependencies based on version, unfortunately.

RiskoZoSlovenska commented 7 months ago

Perhaps we could use https://luarocks.org/modules/siffiejoe/bit32 instead?

rolandlo commented 7 months ago

Perhaps we could use https://luarocks.org/modules/siffiejoe/bit32 instead?

It looks like that would work. For LuaJIT it's much slower (by a factor of roughly 40 in my tests) though than the builtin bit.band and it's also slower (but only by about a factor of 2) than the & operator from Lua 5.3/5.4. So I wouldn't want to use band from bit32 for LuaJIT and Lua 5.3/5.4.

RiskoZoSlovenska commented 7 months ago

Did you benchmark band by itself or in the context of voperation.set and voperation.call (the places where it's used)? As in, is the performance when calling operations actually substantially affected?

rolandlo commented 7 months ago

I benchmarked band by itself. I don't know how much it affects the performance of voperation.set and voperation.call.

RiskoZoSlovenska commented 7 months ago

I'd benchmark the latter before eliminating bit32 as underperformant; in the grand scheme of things, it's the voperation methods that we care about, and bit32's raw performance doesn't matter if it doesn't actually slow them down much.

rolandlo commented 7 months ago

I'd benchmark the latter before eliminating bit32 as underperformant; in the grand scheme of things, it's the voperation methods that we care about, and bit32's raw performance doesn't matter if it doesn't actually slow them down much.

In case the band performance doesn't matter much we could also consider using a hardcoded fallback that is used when neither the bit nor the bit32 modules are installed. Then there wouldn't be any need for a dependency.