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

feat: expose things #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Arrowana
Copy link
Contributor

@Arrowana Arrowana commented Nov 28, 2022

Work from @tommyip and @codewithgun to use serum-dex from a client

I feel those are things everyone reimplement in their own fork

}
}
pub fn iter(&self, descending: bool) -> LeafNodeIterator<'_> {
let mut stack = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it's worth initializing this one with a large enough capacity

Choose a reason for hiding this comment

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

+1 the bump allocator in solana runtime makes dynamic vector resizing deceptively brutal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't see anything in history giving an hint at what vec capacity we want to start with, any hint?

Copy link
Contributor

Choose a reason for hiding this comment

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

Vec::with_capacity(self.header().leaf_count as usize);

leafs: self.slab.iter(is_descending).peekable(),
}
}
}
Copy link

@jarry-xiao jarry-xiao Nov 29, 2022

Choose a reason for hiding this comment

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

Think you can do this in a single expression if you import itertools:

self
    .slab
    .iter(is_descending)
    .map(|leaf| (leaf.price(), leaf.quantity()))
    .group_by(|(p, q) *p)
    .map(|price, group| Level {price, quantity: group.map(|(_, q)| q).sum() })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way i found that would satisfy lifetime requirements is to add an iter function on LevelsIterator, is this how we want it?

pub struct LevelsIterator<'a> {
    leafs: LeafNodeIterator<'a>,
}

impl<'a> LevelsIterator<'a> {
    fn iter(&'a self) -> Box<dyn Iterator<Item=Level> + 'a> {
        Box::new(self.leafs
            .map(|leaf| (leaf.price(), leaf.quantity()))
            .group_by(|(p, q)| *p)
            .into_iter()
            .map(|(price, group)| Level {
                price: price.get(),
                quantity: group.map(|(_, q)| q).sum(),
            }))
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor Author

@Arrowana Arrowana Dec 10, 2022

Choose a reason for hiding this comment

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

I ended up not adding this, i think while more elegant we end up bringing this heap allocation we didn't have before so any program using this will pay. Let me know if you think it is essential

@jarry-xiao jarry-xiao self-requested a review December 9, 2022 03:46
@jarry-xiao
Copy link

AFAICT, these are all non-breaking contract changes and helper utilities. Don't see too much harm in approving

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.

None yet

5 participants