Optimize stack related WCP operations

[OlivierBBB]

No description provided.

[OlivierBBB]

Note. I was initially tempted to optimize things by changing everything to LT. This wouldn't be an optimization since we would convert the check GT( heightNew, 1024 ) (i.e. heightNew > 1024) to LT( 1024, heightNew ) (i.e. 1024 < heightNew). But there can never be collisions with the LT( height, delta ) comparisons since 0 <= delta <= 17.

A consenquence of this is that there can't be any argument collisions between the two use cases. We therefore don't need to distinguish between LT and GT operations in this set. We will be able to get rid of the CHECK_USES_LT and CHECK_USES_GT constants altogether if we so desire.

[OlivierBBB]

Also an option would be to move things from Conflation to TxStack. I'm not sure whether there's any point to it. Indeed the data is pretty "transaction-related."

[OlivierBBB]

@ahamlat : @letypequividelespoubelles suggests splitting the set in 2. It makes sense.

The stack underflow set will always be at most of size 1026 and only needs to record heightNew (also the possible values of heightNew are 0, 1, ..., 1024, 1025.

On the other hand if people may want to practice DUPX/SWAPX scales at all possible heights one could lead to up to (1 + 17) * 1025 = 18450 different elements in the set needed for detecting stack underflows.

Already today both sets of values for SUX/SOX are disjoint but we insert values in the same StackedSet. And @letypequividelespoubelles 's argument is that this way we would have faster search times for finding membership. The only argument against it I can see is having to deal with 2 maps rather than 1, so more complexity. But that's about it.

Should we do it ?

[letypequividelespoubelles]

Should we do it ?

I'm in favor of it, yes ! :)

[OlivierBBB]

It is done @letypequividelespoubelles

[letypequividelespoubelles]

instead of 0 it should return an exception, as this line count shouldn't be called

[ahamlat]

LGTM, just small comments but nothing blocking

[ahamlat]

For my understanding, why the height can go from 0 to 1025 ? I would expect 1024 instead of 1026 elements

[OlivierBBB]

The height is not allowed to go to anything > 1024 but certain instructions will bring it to 1025. E.g. your stack is full (height = 1024) and you do a PUSHX or TIMESTAMP or so ... some instruction that doesn't remove any items, $\mathtt{nbRemoved} = 0$, but adds onto it, $\mathtt{nbAdded} = 1$. Or in EYP terminology $\delta < \alpha$. In this case the zkevm will compute heightNew = height - delta + alpha = height + 1 = 1024 + 1.

[OlivierBBB]

It leads to a stackOverflowException and so it's not a legal value from the EVM pov. From the zkevm pov the WCP module will test whether heightNew > 1024 regardless, in this case the answer would be true and so the zkevm knows to throw a SOX (STACK_OVERFLOW_EXCEPTION).

[ahamlat]

I suggest to use checkState in this case instead of checkArgument. checkArgument will return IllegalArgumentException and it is used usually to check that the argument of a method is well defined. In this case, we are more interested in the state of calculated field. checkState will return IllegalStateException if you use Guava implementation.

[ahamlat]

The same as above

[ahamlat]

We can expose a get method on the hub level to access stackHeightChecksForStackUnderflows data structure. The method in Hub.java would return transients().conflation().stackHeightChecksForStackUnderflows(), if we know that transients() and conflation() will not return null. We can add checks to avoid NullPointerExceptions

[ahamlat]

The same as above

[ahamlat]

Why the check on the result is not needed anymore ?

[OlivierBBB]

Because the replay test wasn't working when I didn't specify false. We have several tests where the state check fails at the end, @DavePearce will know more.

[DavePearce]

But i think it a working now, right?

[OlivierBBB]

I don't know, I didn't test it. I'll do it now 🙂

[lu-pinto]

I would suggest using an Integer as the key to the StackedSet since the only field you're using here is an int. If you have lots of these kinds of checks you will create a new key every time which causes lots of memory churn.

Integer has a cache within the JVM itself from -128 to 127 but it's also allowed to run further with a flag. Taken from java.lang.Integer class:

    private static final class IntegerCache {
        static final int low = -128;
        static final int high;

        @Stable
        static final Integer[] cache;
        static Integer[] archivedCache;

        static {
            // high value may be configured by property
            int h = 127;
            String integerCacheHighPropValue =
                VM.getSavedProperty("java.lang.Integer.IntegerCache.high");
            if (integerCacheHighPropValue != null) {
                try {
                    h = Math.max(parseInt(integerCacheHighPropValue), 127);
                    // Maximum array size is Integer.MAX_VALUE
                    h = Math.min(h, Integer.MAX_VALUE - (-low) -1);
                } catch( NumberFormatException nfe) {
                    // If the property cannot be parsed into an int, ignore it.
                }
            }
            high = h;

            // Load IntegerCache.archivedCache from archive, if possible
            CDS.initializeFromArchive(IntegerCache.class);
            int size = (high - low) + 1;

            // Use the archived cache if it exists and is large enough
            if (archivedCache == null || size > archivedCache.length) {
                Integer[] c = new Integer[size];
                int j = low;
                for(int i = 0; i < c.length; i++) {
                    c[i] = new Integer(j++);
                }
                archivedCache = c;
            }
            cache = archivedCache;
            // range [-128, 127] must be interned (JLS7 5.1.7)
            assert IntegerCache.high >= 127;
        }

        private IntegerCache() {}
    }

[ahamlat]

The StackHeightCheck StackedSet is a small one, the maximum length is 256. I don't think it is worth using the Integer inner cache here.
The cache was specifically created to reduce the auto-boxing overhead of Integer to int, and works only with Integer.valueOf().

[lu-pinto]

I could be wrong but I thought WcpOperations were being created 2x per opcode execution, hence you would do lots of key lookups during tracing. Exactly! My plan was to compute the comparison int and store it in an Integer which would be then used as a cached key. It is a permanent cache so that's a downside.
Anyway this is based on what I thought the behaviour was, maybe it's worth to have another round of performance analysis before doing that

[OlivierBBB]

The reason I'm using a StackHeightCheck class which is just a wrapper around an int is that they will populate two StackedSet's which in turn require as keys things implement the ModuleOperation interface. I wasn't sure whether I can decalre Integer to follow that interface (but I also didn't try.)

[OlivierBBB]

In reality we could extract the StackedSet logic and make it generic. Something that has

• a set of committed objects
• a set of as of current objects
• methods to enter, pop, ...

and then extend it to a more specialized StackedSetOfModuleOperations with more module specific operations like lineCount etc ... @letypequividelespoubelles @ahamlat @lu-pinto what do you think ?

This way we would have a fully general stacked set class which could accept e.g. Integer's.

[OlivierBBB]

The StackHeightCheck StackedSet is a small one, the maximum length is 256

That isn't quite true. For overflows the maximum size of this set is 1026. For underflows the max size is (1 + 17) * 1025 = 18450. I take it the 256 you reference is the initial allocation. Which tbh we may as well raise to 1024.

[letypequividelespoubelles]

n reality we could extract the StackedSet logic and make it generic. Something that has

a set of committed objects
a set of as of current objects
methods to enter, pop, ...

and then extend it to a more specialized StackedSetOfModuleOperations with more module specific operations like lineCount etc ... @letypequividelespoubelles @ahamlat @lu-pinto what do you think ?

Yes, it's the right solution. More we don't need to implement the line counting for the StackedSetOp

[letypequividelespoubelles]

2caecd0

[ahamlat]

I take it the 256 you reference is the initial allocation. Which tbh we may as well raise to 1024.

May be we should review the naming then, because in the constructor, the two arguments are : expectedConflationNumberOperations and expectedTransactionNumberOperations. I would suggest to replace expected with initial.

[lu-pinto]

Sorry somehow missed this yday.

which in turn require as keys things implement the ModuleOperation interface

Alright that makes sense. I completely missed this and just assumed StackedSet was generic. Yes I agree that having a class that is generic and another that extends from it that does more specialized checks on add is the right way to go.