aionnetwork / AVM

Enabling Java code to run in a blockchain environment
https://theoan.com/
MIT License
49 stars 25 forks source link

lazyLoad() calls from constructor can cause unexpected OutOfEnergyException #348

Closed jeff-aion closed 5 years ago

jeff-aion commented 5 years ago

Some of our shadow JCL and API types call lazyLoad() directly from their instance stub constructors. This was done, earlier on, to remove the loading attempt from the critical path of our simpler data types (Integer, etc).

This approach causes 2 problems:

  1. Specification complexity
  2. Complex OutOfEnergyException behaviour

Specification complexity is due to the special-case billing behaviour this describes. Data reads are only billed if an instance variable of an instance is touched (including its identity hash) but these cases are also loaded if they are even referenced. These special-cases would need to be communicated.

OutOfEnergyException is more directly problematic. Normally, we can exhaust energy when loading the data backing an object, based on the type's SerializedRepresentation size. Actually instantiating and populating the object, however, cannot fail for any reason (this is considered fatal to the AVM since any errors at this point would be static bugs in implementation or fatal misconfiguration/JVM assumptions). Herein lies the problem: lazyLoad() can fail with OutOfEnergyException but the instance stub constructor cannot fail for any reason so calling lazyLoad() from that constructor is clearly an error.

jeff-aion commented 5 years ago

Looking at this problem in greater detail, there are a few related issues I have run into which are worth mentioning:

  1. Number is actually what calls lazyLoad() but so do some of its sub-classes (but not all), from within its constructor. This, obviously, needs to be removed as the lazyLoad() calls are pushed down to the points where the instance variables are accessed.
  2. In a few places, public APIs are calling other public APIs (like an instance method calling a static helper), which is generally a bad practice and causes double-billing, in this case. These need to be unwound.

I am making both changes as I progress and will come up with some way to carve up the individual changes into something sensible once complete.

jeff-aion commented 5 years ago

When changing these public APIs which were calling public APIs to flatten (avoid the double-charge), a consistent problem point which is worth pointing out is the CharSequence interface: we must convert it to a shadow String in order to use it in the shadow JCL which does add an additional cost since it calls the user's code to do the conversion.

This seems unavoidable so we probably just need to remember to document it in the spec.