[CLOSED] Add testing capability to Helper

[aionbot]

Issue created by jeff-aion (on Wednesday Nov 07, 2018 at 21:28 GMT)

Helper is the container of our static runtime support methods called from our instrumentation injected into user code, upon deployment.

This has worked well for months but a big limitation is that we can't easily mock it for testing or other execution introspection purposes. Issue #303 is a good example of a testing need which can't be met without changing the Helper and there are several complex testing scenarios involving the concurrent executor which would require this kind of support.

This will be done in 2 phases:

1. Rename the Helper in the injected code, as something which is more obviously "the runtime helper class", transforming the given helper to that name during AVM startup. This means that the Helper can be changed between invocations since it will be using this abstract name, not directly hard-coded.
2. Enable the actual testing case.

This "testing case" requires running a few experiments since we know of at least 2 ways this could go:

• Move the Helper state and implementation into an actual instance (or maybe rationalize it with the IHelper interface), and just call this instance from the Helper static entry-points (so the only static state would be the pointer to the IHelper instance). This would be cleanest but may be expensive in the critical path, so this should be attempted first but measured against our usual perf benchmarks. I suspect that a cost of more than 5% would make this unreasonable (if even that).
• Just inject the Helper implementation bytecode, directly when creating the AvmImpl. This way, we still hit static entry-points but they are in a different implementation. This won't change the critical path (hence, performance unchanged) but doesn't expand capabilities as elegantly: no verification on the Helper required methods (as the interface could provide), extra difficulty involved in "wrapping" or otherwise interacting with the default implementation (it would have to be encapsulated in another class loader), etc.

[aionbot]

Comment by jeff-aion (on Thursday Nov 08, 2018 at 16:51 GMT)

Interestingly, BlockchainRuntime is injected in a similar way to Helper so we can probably generalize it in the same way.

Once the core mechanisms to inject these classes are in place, we can experiment with whether they can be completely turned into instances or if that cost (invokevirtual/invokeinterface inside the static stubs) is too high and they should remain externally-provided classes.

[aionbot]

Comment by jeff-aion (on Friday Nov 09, 2018 at 15:09 GMT)

A possible direction we could take this, if that "Helper instance" idea performs well enough, is that we could do the same treatment to BlockchainRuntime: build in the specific static stubs for both of these helper classes and then pass in instances of each, as specific interfaces, when we decide to execute a transaction.

[aionbot]

Comment by jeff-aion (on Friday Nov 09, 2018 at 20:43 GMT)

I made quick prototype (not all tests pass but most do - I suspect a different "reentrant Helper dance" is required) of the idea that we should use an interface instance, instead of the direct static method, to implement Helper and collected some initial performance data on Sha1PerfTest (average of 5 runs each):

STATIC cached: 203 291 ns STATIC uncached: 1 905 842 ns INTERFACE cached: 216 446 ns (6% slower) INTERFACE uncached: 1 500 873 ns (21% faster)

I will take a deeper look into the shape of the profile and see how stable these numbers are but I suspect that the reason the trend diverges is because of the size of the Helper code: using the interface, this is tiny, meaning that the time spent loading new code is probably much less. After JIT however, the interface is slower since it does represent an additional indirection to the data, even if the caller is fully devirtualized.

If there are no other costs associated with this, I would still recommend that we proceed with the interface approach since the uncached performance will likely start out as the common case and this is a far more elegant and versatile design.

[aionbot]

Comment by jeff-aion (on Friday Nov 09, 2018 at 21:17 GMT)

I did a few 30s profiles of this [uncached] workload, before and after the change, and the assumption of time spent in verification seems consistent. Under AvmImpl.commonInvoke(), these are consistently the top 3 hot spots:

1. DAppExecutor.call() - Actually executes the transaction
2. DAppLoader.loadFromGraph() - Loads the DApp
3. LoadedDApp.cleanForCache() - Runs Helper cleanup routines

Using the static approach, the following timings seemed pretty consistent:

1. 64% - DAppExecutor.call()
2. 33% - DAppLoader.loadFromGraph()
3. 1.5% - LoadedDApp.cleanForCache()

Using the interface approach, the following timings seemed somewhat consistent:

1. 71% - DAppExecutor.call()
2. 23% - DAppLoader.loadFromGraph()
3. 3.5% - LoadedDApp.cleanForCache()

Note that the interface approach also has no optimization and is just a coarse prototype. A real implementation would also involve changes to IHelper, avoiding additional hops between the static and instances. This probably won't make core code much faster but may render the cleaning operation much faster.

This data seems to confirm the previous suspicions and implies that we should proceed with this implementation. Worst case scenario, we can always revisit this and re-inline the instance into the class but I suspect that the benefit we will derive from the simplicity of this model will open new opportunities to simplify or optimize the system, in the interim.

[aionbot]

Comment by jeff-aion (on Monday Nov 12, 2018 at 21:52 GMT)

Part of the difficulty with the current IHelper design (and why we want to replace it), is that it ties together the state of a specific DApp's execution with the ability to call code associated with the current DApp from anywhere in our API.

While these are related, we actually treat them quite differently and how we ensure that this works for the reentrant case, as well as some of our tests, shows the difficulty in formalizing these approaches: there are many cases where the thread local is manipulated from outside a formalized routine. The reentrant case also handles the stack of currently executing DApp states in a rather convoluted way: asking the IHelper to capture the static state snapshot into its instance state, then swapping it out to install a new instance.

Instead, my current thinking is to explicitly require that a thread running a DApp has first "attached" to the VM (and must "detach" when done). This attachment will involve associating a [potentially long-lived] interface with the thread, which will implement all calls required to support the instrumentation and API, internally including the stack of DApp states the associated thread is currently running.

When calling into a new DApp, any currently-running DApp's runtime static is set to null, the thread's interface is told to push a new state frame, the target DApp's runtime static is set to the thread's interface, and the DApp is invoked. Upon return, the target Dapp's runtime static is set to null and the thread's interface is told to pop its state frame. If there this was a call from a DApp, its runtime static is set to the thread's interface and its execution is resumed. Note that some state may need to commit from the callee into the caller (next hash code, for example).

This approach will avoid the current confusion regarding where the call stack data is and who is responsible for clearing it, how/when the IHelper is instantiated/restored, and who manages the thread local in the thread. It will, however, be a large change since many of our tests currently interact with this but will need to change to the attach/detach model.