Skip to content

Conversation

@tekknolagi
Copy link
Contributor

@tekknolagi tekknolagi commented Oct 30, 2025

Per #15002, this happens frequently in benchmarks.

struct.c Outdated
}

bool
rb_yarv_struct_instances_embedded_p(VALUE klass)
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'm not sure this is the best way to do this. YJIT does it by inspecting an instance, which we don't have. Maybe we could add it to profiles? :/

Copy link
Member

@XrXr XrXr Oct 30, 2025

Choose a reason for hiding this comment

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

Can't you use recv_type.flags().is_embedded()?
EDIT: nevermind it's a different bit for structs than T_OBJECTs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. I thought that was only for ivars. If not then yes we can absolutely do that

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 am adding a new bit

zjit/src/hir.rs Outdated
.try_into()
.unwrap();
// Confidence checks
// assert!(unsafe { RB_TYPE_P(recv, RUBY_T_STRUCT) });
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 would like to figure out a way to keep these kinds of checks in ZJIT

@tekknolagi tekknolagi marked this pull request as ready for review October 30, 2025 21:01
@matzbot matzbot requested a review from a team October 30, 2025 21:02
struct.c Outdated
}

bool
rb_yarv_struct_instances_embedded_p(VALUE klass)
Copy link
Member

@XrXr XrXr Oct 30, 2025

Choose a reason for hiding this comment

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

Can't you use recv_type.flags().is_embedded()?
EDIT: nevermind it's a different bit for structs than T_OBJECTs.

@tekknolagi tekknolagi enabled auto-merge (rebase) October 30, 2025 22:01
@tekknolagi tekknolagi merged commit f8b4feb into ruby:master Oct 30, 2025
86 checks passed
@tekknolagi tekknolagi deleted the mb-opt-struct-aref branch October 30, 2025 22:31
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