Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Events Orderbook] Implement copy on write for latest auction state #734

Open
nlordell opened this issue May 6, 2020 · 4 comments
Open
Labels
enhancement New feature or request

Comments

@nlordell
Copy link
Contributor

nlordell commented May 6, 2020

The streamed orderbook works by keeping a "confirmed" auction state that only includes confirmed events that are unlikely to ever change because of reorgs, and a "latest" state that include events up until the latest block.

The "latest" state currently is calculated by doing a deep clone of the "confirmed" state and then applying the events between the confirmed block and the latest block. Because of potential reorgs, the "latest" state needs to be rebuilt on every update, which includes the deep clone of the "confirmed" state. This is inefficient given that there are usually a relatively small number of "latest" events that only affect a small part of the state.

This issue captures an enhancement to implement copy on write semantics for the auction state where the "latest" state will keep only a shallow copy of the data until a part needs to be modified, at which point it will do a deep clone of only that account/order/pendingwithdrawal/etc. This will allow the "latest" state to be created much more efficiently and avoid unnecessary deep clones.

@fleupold
Copy link
Contributor

fleupold commented May 6, 2020

Do we know how long the deep copy actually takes (e.g. compared to fetching the events since the last confirmed block)? This would help us prfioritize this task.

@nlordell
Copy link
Contributor Author

nlordell commented May 6, 2020

Do we know how long the deep copy actually takes (e.g. compared to fetching the events since the last confirmed block)?

I imagine it is a very small amount of time, I don't even see a spike in my CPU usage when doing an update. I can get some numbers if you like.

This would help us prfioritize this task.

I definitely do not think this is a priority. I think the biggest issue this causes is it allocates and throws out a lot of garbage, which may eventually cause longer garbage collection cycles periodically. I think given the scale of our price estimator service, I don't imagine this being an immediate issue.

@fleupold
Copy link
Contributor

fleupold commented May 6, 2020

My concern is if this is worth doing at all.

@nlordell
Copy link
Contributor Author

nlordell commented May 6, 2020

update: Ran an unscientific benchmark like so:

const start = require("perf_hooks").performance.now()
for (let i = 0; i < 1000; i++) {
  (orderbook as any).confirmedState.copy()
}
console.log(require("perf_hooks").performance.now() - start)

And it took 9358.11694300174ms, so roughly 10ms per copy.

I think in the long term it might be a nice optimization and I don't think it would be too much work. It would also allow us to lazily apply events on query and change which events get applied, depending on whether we want the open or the finalized orderbook.

nlordell added a commit that referenced this issue May 6, 2020
…ck (#735)

This PR implements querying the latest open orderbook.

This is implemented by keeping a separate "confirmed" auction state, built from events that are part of confirmed blocks, and not subject to reorgs, and a "latest" auction state that is built by cloning the "confirmed" state and applying events in between the confirmed block and the latest block.

The "latest" state is implemented to be discardable and rebuilt on every update so that it is not prone to errors stemming from reorgs, nor does complex logic for rolling back events need to be implemented. Unfortunately, the current implementation is slightly inefficient. I created #734 as an issue to capture a potential solution to the problem.

This closes #719 

### Test Plan

Run the e2e test to ensure the event based orderbook on the latest block matches the onchain queried orderbook:
```
$ yarn test-streamed-orderbook

  Streamed Orderbook
    init
==> building streamed orderbook...
==> querying onchain orderbook...
==> comparing orderbooks...
      ✓ should successfully apply all events and match on-chain orderbook (287940ms)


  1 passing (5m)
```


* implement querying the orderbook at the latest block

* comments

* add unit test for copy

* fix array index logic

* default to system time for future blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants