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

Fix microbenchmarks #361

Merged
merged 7 commits into from
Dec 6, 2022
Merged

Fix microbenchmarks #361

merged 7 commits into from
Dec 6, 2022

Conversation

antocuni
Copy link
Collaborator

Microbenchmarks got slightly out of date and didn't build, fix it and improve the readme

@hodgestar
Copy link
Contributor

Should we add running the microbenchmarks to CI so that they don't break in future?

Copy link
Contributor

@fangerer fangerer left a comment

Choose a reason for hiding this comment

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

I think this micro benchmark could use a further update.
Function allocate_tuple_impl says:

HPyErr_SetString(ctx, ctx->h_Exception, "HPy_BuildValue not implemented yet");

but that's no longer true. We already have it.
But I'm already fine with merging that.

@@ -65,7 +65,8 @@ static HPy allocate_tuple_impl(HPyContext *ctx, HPy self)
/* Foo type */

typedef struct {
PyObject_HEAD
long x;
long y;
} FooObject;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we even need FooObject? An HPy type can also have .basicsize = 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's because we wanted to have non-0 allocations in test_allocate_*, they are more likely to be representative.

@antocuni
Copy link
Collaborator Author

I think this micro benchmark could use a further update. Function allocate_tuple_impl says:

HPyErr_SetString(ctx, ctx->h_Exception, "HPy_BuildValue not implemented yet");

but that's no longer true. We already have it. But I'm already fine with merging that.

ah good point. I'll try to fix that

@antocuni
Copy link
Collaborator Author

Should we add running the microbenchmarks to CI so that they don't break in future?

yes, good idea

@fangerer fangerer merged commit c7b51e7 into master Dec 6, 2022
@fangerer fangerer deleted the antocuni/fix-microbenchmarks branch December 6, 2022 13:57
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

3 participants