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

bench action: no codegen, but run lscpu #74

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

mumbleskates
Copy link
Contributor

@mumbleskates mumbleskates commented Apr 8, 2024

it is no longer necessary to fetch codegen tools, as they are no longer needed by default since #68

however, at the level these libraries often operate, the runtime CPU matters. github actions' hosting infrastructure is pretty mysterious, so maybe we can pull the veil back a little bit

(i'm seeing regressions in the bilrost "prepend" path for a couple of these benchmarks; this is nothing new, it's actually amazing how much of the modern software & hardware stack seemingly revolves around the assumption that nobody does this)

it is no longer necessary to fetch codegen tools, as they are no longer needed by default since 1a92464a3d39be45cf5b96af55f211991f70359e

and at the level these libraries often operate, the runtime CPU matters. github actions' hosting infrastructure is pretty mysterious, so maybe we can pull the veil back a little bit
@djkoloski
Copy link
Owner

I think this is a good change to make. We should include this in our benchmark data as a metadata field and use it when generating the README.

@mumbleskates
Copy link
Contributor Author

are there more supporting changes to make here to format that into the readme? i have not delved into the tools folder much at all

@djkoloski
Copy link
Owner

You're welcome to make those changes if you want, but I can also just make them into an issue for future work.

Generally, the bencher crate orchestrates benching and gathering all the data. It calls out to the parser crate to turn the raw bench output into JSON, then the formatter crate to generate the README using that JSON. The JSON format is specified by the schema crate, which is shared between the parser and formatter.

@mumbleskates
Copy link
Contributor Author

okay! that's a great guideline, thanks; i'll see if i can just add a field we pass through and try to pipe some lscpu output into it.

Do you feel this change can be merged independently of that? I'm ambivalent. I'm mildly curious what these machines even are but only mildly; i think i can infer that they are Xeons purely based on the comparative performance of my code (yeah, really lol; something about the µcode generation is reliably significantly different between the two; AMD cpus are almost always faster in prepend, but i have not been able to get quite the same wins out of Intel yet). It'd be cool to have that in the actions logs but maybe it's not worth making this change until it actually shows up in the output.

@djkoloski
Copy link
Owner

Yeah we can merge this right now. Just mark it as no longer a draft and I'd be happy to merge it.

@mumbleskates mumbleskates marked this pull request as ready for review April 10, 2024 22:25
@mumbleskates
Copy link
Contributor Author

marked

@djkoloski djkoloski merged commit d0328b2 into djkoloski:master Apr 10, 2024
1 check passed
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.

2 participants