CC: Tweaked

CC: Tweaked

65M Downloads

[API Proposal]: Java API for Lazy Array/Table

MineCake147E opened this issue ยท 6 comments

commented

Background and motivation

Currently, CobaltLuaMachine.toValue, when it gets a value returned from a @LuaFunction, it seems to enumerate all elements of a given Map<?, ?> or Collection<?>, creating Lua objects for each element.
Running such code in the main thread would hurt tick performance, which leads to a server lag.
I think it's nice to have some additional APIs for cached lazy table/array objects in Java territory, in order to improve performance in the main thread.

API Proposal

package dan200.computercraft.api.lua;

import it.unimi.dsi.fastutil.ints;

public interface ILazyLuaArray {
    /*
     * Java API for replacing the `__len` metamethod.
     * Once this method returns any value, the return value will be cached.
     */
    int getLength();

    /*
     * Java API for replacing the `__pairs` metamethod.
     */
    default Iterable<Int2ObjectMap.Entry<Object>> getPairs(ILuaContext context, int index) {
        // Default implementation to be supplied
    }

    /*
     * Java API for replacing `__index` metamethod.
     * Once this method returns non-null value, the return value will be cached.
     */
    @Nullable
    Object constructValueAt(ILuaContext context, int index);
}

public interface ILazyLuaTable {
    /*
     * Java API for replacing the `__pairs` metamethod.
     */
    Iterable<Map.Entry<Object, Object>> getPairs(ILuaContext context, int index);

    /*
     * Java API for replacing `__index` metamethod.
     * Once this method returns non-null value, the return value will be cached.
     */
    @Nullable
    Object constructValueAt(ILuaContext context, Object index);
}

API Usage

/* Intended to be used in something like `InventoryMethods.list()`. */
public final class LazyItemList implements ILazyLuaArray {
    Int2ObjectSortedMap<ItemStack> slots;
    final int slotCount;

    public LazyItemList(IItemHandler inventory) {
        var s = inventory.getSlots();
        slotCount = s;
        slots = new Int2ObjectLinkedOpenHashMap();
        for (int i = 0; i < s; i++) {
            ItemStack stackInSlot = inventory.getStackInSlot(i);
            if (stackInSlot == null || stackInSlot.isEmpty()) continue;
            slots.put(i, stackInSlot.copy());
        }
    }

    public int getLength() {
        return slotCount;
    }

    @Nullable
    public Object constructValueAt(ILuaContext context, int index) {
        var stack = slots.getOrDefault(index, ItemStack.EMPTY);
        return !stack.isEmpty() ? VanillaDetailRegistries.ITEM_STACK.getBasicDetails(stack) : null;
    }
}

Alternative Designs

constructValueAt might have to return MethodResult instead of Object.

Risks

None

commented

I think turn Map to LuaTable lazely is acceptable and may have a little bit performance improvement.
However, make constructValueAt or getPairs throws LuaException is not reasonable. They should not have any side effect but only construct LuaValues lazely.

commented

However, make constructValueAt or getPairs throws LuaException is not reasonable. They should not have any side effect but only construct LuaValues lazely.

I assumed that it was necessary because IDynamicLuaObject had such function. Correct me if I'm wrong.

commented

However, make constructValueAt or getPairs throws LuaException is not reasonable. They should not have any side effect but only construct LuaValues lazely.

I assumed that it was necessary because IDynamicLuaObject had such function. Correct me if I'm wrong.

IDynamicLuaObject's only method that throws LuaException is:

MethodResult callMethod(ILuaContext context, int method, IArguments arguments) throws LuaException;

which only throws error when you call a method. It is reasonable because you are going to do some blackbox operations that may fail internally. But your API proposal is going to throw exceptions while accessing values, for people who do not know the internal logic well, they will feel it really weird since it does not match their common sense.


Just to clarify, in CC:T all returned values are safe to access, because they had already been serialized once.
Your API proposal did nothing wrong on Lua logic. However, If your goal is only improve the performance when accessing, but not do lazy operations that has side effects, then it is better to keep the access from throwing errors.

commented

The main comment I would make is that this seems have a pretty large cost on the Lua agnosticism of CC:T's LuaFunctions (currently the most lua specific things are that tables are associative arrays and that you can return LuaFunctions).
The second one is that I'm just not sure how useful this is. Generally when you are using a peripheral you are using it to get data that isn't owned by the computer, not to perform an expensive calculation (in which case, why not just avoid returning all the junk data in the first place instead of lazy loading it). What would be an actual use case of this feature?

commented

Just to clarify, CobaltLuaMachine.toValue shouldn't ever be called on the main thread.

I misunderstood your code. My bad.

  • You now need to take defensive copies of everything. For instance, that stackInSlot.copy() is probably more expensive for items with NBT (at least on 1.20.1, it'll be better on 1.21) than getBasicDetails!

I didn't know that it's so expensive on 1.20. Thank you for telling me that.

commented

Currently, CobaltLuaMachine.toValue, when it gets a value returned from a @LuaFunction, it seems to enumerate all elements of a given Map<?, ?> or Collection<?>, creating Lua objects for each element. Running such code in the main thread would hurt tick performance

Just to clarify, CobaltLuaMachine.toValue shouldn't ever be called on the main thread. Do you have any profiles where this is causing issues?, I'd definitely be interested in seeing them!

The tricky thing with lazy-loading is that a lot of this work is scheduled on the main thread because it needs to be! If you want to move more work back to the computer thread (with constructValueAt), then you hit a few problems:

  • You're quite limited by what functions you can safely call (for instance, you can call VanillaDetailRegistries.ITEM_STACK.getBasicDetails, but not getDetails).
  • You now need to take defensive copies of everything. For instance, that stackInSlot.copy() is probably more expensive for items with NBT (at least on 1.20.1, it'll be better on 1.21) than getBasicDetails!

I think a more general solution here would probably just be to support arbitrary userdata (dan200/ComputerCraft#173), but have struggled to find a good way to design that which works with CC:T's existing abstractions.