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

Port Portfolio to Rust #2058

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Pushkarm029
Copy link
Collaborator

Pull Request

Include a summary of the changes.

Type of change

Delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this change been tested?

Describe how this code was/is tested.

Copy link

codspeed-hq bot commented Nov 13, 2024

CodSpeed Performance Report

Merging #2058 will improve performances by 75.33%

Comparing Pushkarm029:portfolio2rs (5508ea1) with develop (94134e9)

Summary

⚡ 1 improvements
✅ 51 untouched benchmarks

Benchmarks breakdown

Benchmark develop Pushkarm029:portfolio2rs Change
test_condition_none 22.4 µs 12.7 µs +75.33%

Copy link
Collaborator

@twitu twitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw why do we need the pub type Statistic to be Arc<dyn ...> when it's supposed to be used single threaded?


// todo
// Initialize initial (order) margin
// let result_init = self.accounts.
}

pub fn update_account(&mut self, event: &AccountState) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_account only needs a reference to the cache. It does not need access to the full state of the Portfolio. We can avoid the inner outer pattern by moving a clone of the cache the update account closure. Just like we did message bus in RiskEngine.

@faysou
Copy link
Collaborator

faysou commented Nov 20, 2024

@Pushkarm029 you can rebase your dev branch and do a force push instead of merging with the dev branch which adds a lot of unnecessary commits.

I use this shell script which is quite practical, it has an optional argument to reset one or more commits as well (I like to see my current differences and edit them as well, instead of just adding commits, which is ok for pull requests):

custom_rebase() {
    git fetch origin develop:develop

    git stash
    git rebase develop
    git stash pop

    local commits=${1:-0}
    if [ "$commits" -gt 0 ]; then
        git reset --soft HEAD~"$commits"
    fi
}

Signed-off-by: Pushkar Mishra <[email protected]>
@Pushkarm029
Copy link
Collaborator Author

Sorry to everyone who got notified; it’s fixed now.😅

Copy link
Collaborator

@twitu twitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, we need the inner outer pattern for some of the methods.

fn register_message_handlers(
msgbus: &Rc<RefCell<MessageBus>>,
cache: &Rc<RefCell<Cache>>,
portfolio: &Rc<RefCell<Self>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot create an rc refcell of self from inside the new function. We will have to use an inner outer pattern here.

Also no need to pass rc refcell by & reference. Pass it by cloning it.

}

pub fn update_quote_tick(&mut self, quote: &QuoteTick) {
todo!()
self.unrealized_pnls.remove(&quote.instrument_id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update quote tick uses self fields that are not behind and Rc Refcell which is why it won't work by just passing the cache or the message bus. The other update functions too use parts of self that are not cloneable references. So it seems like an inner outer style must be implemented.

Comment on lines +152 to 156
accounts: AccountsManager,
analyzer: PortfolioAnalyzer,
// venue: Option<Venue>, // Added for completeness but was meant to be a "temporary hack"
unrealized_pnls: HashMap<InstrumentId, Money>,
realized_pnls: HashMap<InstrumentId, Money>,
net_positions: HashMap<InstrumentId, Decimal>,
Copy link
Collaborator

@twitu twitu Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps for portfolio we can take a slightly different approach to inner outer. These lines are the actual inner state of the portfolio. Perhaps they can be wrapped into their own struct, and rc ref celled on this Portfolio struct (which will act as outer).

struct PorfolioState {
    accounts: AccountsManager,
    analyzer: PortfolioAnalyzer,
    unrealized_pnls: HashMap<InstrumentId, Money>,
    realized_pnls: HashMap<InstrumentId, Money>,
    net_positions: HashMap<InstrumentId, Decimal>,
}

struct Porfolio {
  ...
  inner. Rc<RefCell<PortfolioState>>

This way you can clone the portfolio state and pass it to the closure and relevant functions without needing to wrap the whole portfolio struct which might be cumbersome. However, it will mean that all the functions that the closures call will have to be changed slightly to receive the portfolio state, message bus, cache as separate parameter.

update_quote_tick(&self, ,.....)
update_qutote_tick(cache, messagebus, portfolio_state, ....)

To abstract away the borrow and borrow mut calls. You can also consider an additional thin helper layer.

struct PorfolioWrapper {
   inner: Rc<RefCell<PortfolioState>>,
}

struct Porfolio {
    inner: PortfolioWrapper
 }

For PortfolioWrapper you can consider implementing something like these helper methods to easily access the inner fields like

impl PortfolioWrapper {
    fn accounts(&self) -> impl Deref<Target = &AccountsManager> {
        self.inner.borrow().accounts
    }
}

Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants