Skip to content

Wrap analyzeme 0.7.0 and 9.0.0 under a single API #788

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

Closed
wants to merge 1 commit into from

Conversation

lqd
Copy link
Member

@lqd lqd commented Oct 29, 2020

This PR tries to wrap both versions of analyzeme used by the site to process the self-profile data more easily.

As we discussed as a first WIP step, cargo check-quality: I was unable to test it, so I preferred opening a draft PR. I made assumptions on being able to infer the version from the names of files in the tarballs, so I hope it works.

I've added both dependencies: the same 0.7.1 revision used before the PR, and master, as the released 9.0.0 doesn't contain your recent paged_buffer changes.

@rylev
Copy link
Member

rylev commented Jul 9, 2021

Going to close since this is a bit stale. Feel free to reopen if you'd like to continue working on this (or if you think this should be rebased and re-reviewed).

@rylev rylev closed this Jul 9, 2021
@lqd lqd deleted the analyze_this branch July 9, 2021 09:54
@tgnottingham
Copy link
Contributor

@lqd, do you want to continue forward with this? If so, I can help review. If not, I might take inspiration from it. I had implemented #978 without seeing your PR first, but yours is a more complete solution.

@lqd
Copy link
Member Author

lqd commented Aug 19, 2021

IIRC the latest status was: the APIs were unified and both worked at a low-level, but the high-level entry points, eg differentiating between the two different formats and stored archive files, were not fully done yet (because I didn't know how this worked and had no way of testing at the time).

(There's currently also an incorrect assumption in the PR, about which version to use depending on the layout of the archive file, but that can be easily fixed)

@Mark-Simulacrum hoped to help for the last few bits required to land this, if time permitted, but we never did do so.

If either of you have time to help with this, that'd be cool. Feel free to review, take these commits, or tell me what to do to complete this. I really want to have the processed links back on perf.rlo :)

@tgnottingham
Copy link
Contributor

Well, #978 ended up getting merged, so the processed links work again. :) The old format isn't supported anymore, though. I think that's unlikely to be a problem in practice, but if it becomes one, we can address it.

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