-
Notifications
You must be signed in to change notification settings - Fork 115
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
When generating a flamegraph programmatically the flamegraph::Options::consistent_palette
option shouldn't require filesystem access
#73
Comments
@JordiChauzi this is probably your domain. Any ideas for how we might do this? Maybe the trick is going to be to have the CLI frontend open the file, and then have the |
I haven't looked at the code, but a quick suggestion - passing a |
I think trait PaletteMap {
fn lookup(&self, func: &str) -> (u8, u8, u8);
fn remember(&mut self, func: &str, color: (u8, u8, u8));
fn save(self);
} |
But then |
Yeah, you're right, hence my updated suggestion of having |
There are a few drawback that I can see:
|
I think the details have to be exposed to the user regardless if we want them to be able to choose how to represent the map? Specifically, the trait allows the caller to choose the storage format, and doesn't tie them to using I think it'd be implemented for essentially True, |
What about:
This let the user chooses if and when they want to save the |
This is a fair point, but I don't think it's really useful to let the user choose how the map should be represented at runtime - virtually everyone's going to use a In general I'm a fan of APIs which are minimal, hide their implementation details, and are forward compatible so that the details of how they work and/or extra customization points can be gradually exposed without breaking backwards compatibility and without impacting the users which do not need those extra features. Consider the following: struct PaletteMap { /* ... */ }
// Minimal interface:
impl PaletteMap {
pub fn new() -> Self;
pub fn from_stream(fp: &dyn mut io::Read) -> io::Result<Self>;
pub fn to_stream(&self, fp: &dyn mut io::Write) -> io::Result<()>;
}
// We could add some extra convenance methods if needed:
impl PaletteMap {
pub fn load_from_file(path: &dyn AsRef<Path>) -> io::Result<Self>;
pub fn save_to_file(&self, path: &dyn AsRef<Path>) -> io::Result<()>;
}
// We could also add APIs for inspecting and modifying the palette:
type Color = (u8, u8, u8);
impl PaletteMap {
fn iter(&self) -> impl Iterator<Item=(&str, Color)>
fn get(&self, func: &str) -> Color;
fn set(&mut self, func: &str, color: Color);
}
// And it's simple to use:
let mut palette = PaletteMap::new();
// Generate multiple flamegraphs:
let mut options = Options::default();
options.palette = Some(&mut palette);
flamegraph::from_sorted_lines(options, lines_1, writer)?;
let mut options = Options::default();
options.palette = Some(&mut palette);
flamegraph::from_sorted_lines(options, lines_2, writer)?;
// Now the user can save it:
palette.to_stream(&mut fp)?; This has also the nice property that it's extensible, e.g. along with the color we could add the ability to specify the font family in the future. With this kind of API you can simply add it in a fully backwards compatible manner, since you didn't directly expose the internals.
The user is not supposed to peek at what exactly that string is! (:
But don't we already have that implicitly once |
I agree wholeheartedly with that entire post @koute :) Let's go for that approach! @JordiChauzi do you want to give this one a stab? Or maybe @koute would? I'm a bit strapped for time atm, but would be happy to review. |
Unless, @koute want's to do it, i'll work on it in the following days. |
There should be an API which allows the user to enable the
consistent_palette
option and maintain the palette map themselves in memory without touching the filesystem.The text was updated successfully, but these errors were encountered: