Step 6: kernel integration (process fvm transactions in bulk) #795

aionick · 2019-01-28T20:52:04Z

The Fast VM now processes transactions in bulk.

The commit that introduces the bulk processing logic is 382c96a

The remaining commits relate to a bug fix. Essentially, the contract details were not being deeply copied when one repository flushed to another, which allows for a critical object reference to leak across sibling repositories. A deep copy is now being performed, and some tests added.

The option to flush a deep copy as opposed to the regular flush is separate for now because it comes with a performance penalty, of course, and because it is only required when a sibling that wants to hold on to its state flushes. This happens only at one particular point, in the FastVirtualMachine class.

See here for the FVM changes.

- That is, the AionBlockchainImpl class is now sending its transactions in bulk into the executor.

- AionRepositoryCache now has a flushCopiesTo() method that allows for 'sufficiently deep' (this term is defined in the method documentation) copies of the state of the repository to be flushed to its target. - This method is essential for when a repository will flush to a parent that will create further children before the current repository will fully flush its changes all the way to the database.

- The AionRepositoryCache.flushCopiesTo() method that was introduced in the previous commit is now tested here.

- The critical flushing point in the FastVirtualMachine now uses the flushCopiesTo() method to ensure that copies of the state are flushed to the parent instead of the original references (in certain cases).

- The bridge contract was not re-aligning its storage bytes to be 16 or 32 bytes long, which is now its responsibility. This is just a simple change to make that happen.

aion-kelvin

I don't see any big problems that should block this from shipping ... but there's a few design decisions that I think should re-visited after we get AVM initially integrated in a functioning way.

aion-kelvin · 2019-01-28T22:44:34Z

modAion/src/org/aion/zero/db/AionContractDetailsImpl.java

@@ -344,4 +345,68 @@ public IContractDetails getSnapshotTo(byte[] hash) {

        return details;
    }
+
+    /**
+     * Returns a sufficiently deep copy of this contract details object.


Hey, so we already talked about this in-person, but just writing it here to keep a record:

I think this copy() implementation is ok for now to just get things working. Some not so nice stuff about it:

'sufficiently deep copy' is a bit nebulous (hard to understand intent)

the work of copying primitives, calling clones/copy-constructors of classes is repetitive and error-prone, especially when/if the classes inside change over time

each cloneable class needs to have its own clone method, with its own implementation

Another way to deep-copy would be to use Java's Serializable interface to serialize an object to bytes, then create a new object, its clone, from those bytes. Benefits if we apply this to all cloneable classes:

instead of using the 'sufficiently deep' cloning definition, it is clear what gets serialized, because that's dictated by Java's serialize mechanism (unless overridden), and that will be consistent across all classes we need to clone/serialize

don't need to manually write clone code for all cloneable things (less likely to make "copied reference instead of cloned" mistakes)

(maybe) faster (according to a source from 11 years ago)

seems like other people do this too, as the Apache Commons library has a clone() utility method specifically for this purpose: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/SerializationUtils.html#clone-T-

It is probably too much work to change as part of this PR, but I think it's worthwhile to consider for how we want to do cloning in our code for the future.

aion-kelvin · 2019-01-28T22:58:57Z

modAion/src/org/aion/zero/db/AionRepositoryCache.java

+            // determine which accounts should get stored
+            HashMap<Address, AccountState> cleanedCacheAccounts = new HashMap<>();
+            for (Map.Entry<Address, AccountState> entry : cachedAccounts.entrySet()) {
+                AccountState account = entry.getValue().copy();


can entry.getValue return null?

yeah I'd prefer to make as few assumptions as possible, so good catch, I'll fix this

aion-kelvin · 2019-01-28T22:59:30Z

modAion/src/org/aion/zero/db/AionRepositoryCache.java

+            HashMap<Address, AccountState> cleanedCacheAccounts = new HashMap<>();
+            for (Map.Entry<Address, AccountState> entry : cachedAccounts.entrySet()) {
+                AccountState account = entry.getValue().copy();
+                if (account != null && account.isDirty() && account.isEmpty()) {


account won't be null unless entry.getValue().copy() returns null, which would be weird

I'll fix that up to test out the null on entry.getValue()

aion-kelvin · 2019-01-28T23:01:10Z

modAion/src/org/aion/zero/db/AionRepositoryCache.java

+            }
+            // determine which contracts should get stored
+            for (Map.Entry<Address, IContractDetails> entry : cachedDetails.entrySet()) {
+                IContractDetails ctd = entry.getValue().copy();


same question as above, but this time about getValue/null and ctd != null

aion-kelvin · 2019-01-28T23:05:09Z

modAionImpl/src/org/aion/zero/impl/AionBlockchainImpl.java

+     * This "work" is specific to the {@link AionBlockchainImpl#applyBlock(IAionBlock)} method.
+     */
+    private static PostExecutionWork getPostExecutionWorkForApplyBlock() {
+        return (topRepository,


Could also do this to avoid constantly creating objects for the lambda every time this is called:

Make an instance of PostExecutionWork

Your function goes into PostExecutinWork instead of in that lambda

This method returns that instance all the time

That's true, though a stateless lambda is detected in the invokedynamic bootstrap phase and the exact same instance is inlined each time, it won't get created anew. It's actually faster or equivalent to go with a stateless lambda than an inner/anonymous class

aion-kelvin · 2019-01-28T23:22:09Z

modMcf/src/org/aion/mcf/trie/SecureTrie.java

+
+            secureTrieCopy.setPrevRoot(previousRootCopy);
+            secureTrieCopy.setRoot(rootCopy);
+            secureTrieCopy.setCache(super.getCache().copy());


seems kind of odd to clone a cache (i'd expect to leave it empty and let it get filled up naturally).. since this code already works, I don't have a problem with it. it might be worth looking into later if/when we try to optimize performance (it wouldn't be very productive to clone to cache if the user of the cloned cache just immediately accesses only values not in the cache and everything gets evicted shortly after the cache has been cloned)

that's a good point, I wasn't too sure about this one, but wanted to err on the side of caution since I didn't understand all the implications of the trie, but this decision should probably be confirmed at some time

AionJayT

LGTM.
However, after the integration is done. Need to do some performance analysis between VM and DB.
I doubt the integration actually make the DB performance slower cause a lot of flush and copy happening.

AlexandraRoatis · 2019-01-29T15:55:12Z

modAionImpl/src/org/aion/zero/impl/blockchain/AionImpl.java

        return (r, c, s, t, b) -> {
-            return 0L;


it's weird that the 0L was changes here, but not below

AlexandraRoatis · 2019-01-29T15:55:46Z

modAionImpl/src/org/aion/zero/impl/AionBlockchainImpl.java

-
+        ExecutionBatch batch = new ExecutionBatch(block, block.getTransactionsList());
+        BulkExecutor executor =
+            new BulkExecutor(


incorrect formatting

aionick added 5 commits January 28, 2019 15:30

BulkExecutor now being used properly.

382c96a

- That is, the AionBlockchainImpl class is now sending its transactions in bulk into the executor.

Added tests for the flushCopiesTo() method

b521da7

- The AionRepositoryCache.flushCopiesTo() method that was introduced in the previous commit is now tested here.

Fix flushTo bug in FastVirtualMachine

17c95d3

- The critical flushing point in the FastVirtualMachine now uses the flushCopiesTo() method to ensure that copies of the state are flushed to the parent instead of the original references (in certain cases).

Fix to bridge contract data alignment

1a3d58e

- The bridge contract was not re-aligning its storage bytes to be 16 or 32 bytes long, which is now its responsibility. This is just a simple change to make that happen.

aionick added this to the 0.3.3 milestone Jan 28, 2019

aionick requested review from AlexandraRoatis, AionJayT and aion-kelvin January 28, 2019 20:52

aionick mentioned this pull request Jan 28, 2019

Step 6: kernel integration (process fvm transactions in bulk) aionnetwork/aion_fastvm#47

Merged

aionick changed the title ~~Step 7: kernel integration (process fvm transactions in bulk)~~ Step 6: kernel integration (process fvm transactions in bulk) Jan 28, 2019

aion-kelvin approved these changes Jan 28, 2019

View reviewed changes

AionJayT approved these changes Jan 29, 2019

View reviewed changes

AlexandraRoatis approved these changes Jan 29, 2019

View reviewed changes

aionick changed the base branch from incremental-pr5 to master-pre-merge January 29, 2019 16:25

AlexandraRoatis merged commit 9261224 into master-pre-merge Jan 29, 2019

AlexandraRoatis deleted the bulk-executor-pr branch February 11, 2019 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step 6: kernel integration (process fvm transactions in bulk) #795

Step 6: kernel integration (process fvm transactions in bulk) #795

aionick commented Jan 28, 2019 •

edited

Loading

aion-kelvin left a comment

aion-kelvin Jan 28, 2019

aion-kelvin Jan 28, 2019

aionick Jan 29, 2019

aion-kelvin Jan 28, 2019

aionick Jan 29, 2019

aion-kelvin Jan 28, 2019

aion-kelvin Jan 28, 2019

aionick Jan 29, 2019

aion-kelvin Jan 28, 2019 •

edited

Loading

aionick Jan 29, 2019

AionJayT left a comment

AlexandraRoatis Jan 29, 2019

AlexandraRoatis Jan 29, 2019

Step 6: kernel integration (process fvm transactions in bulk) #795

Step 6: kernel integration (process fvm transactions in bulk) #795

Conversation

aionick commented Jan 28, 2019 • edited Loading

aion-kelvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aion-kelvin Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AionJayT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aionick commented Jan 28, 2019 •

edited

Loading

aion-kelvin Jan 28, 2019 •

edited

Loading