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

Merge Preparation: Merge Precise Object Tagging to Graph Copy EOP #4567

Conversation

luc-blaeser
Copy link
Contributor

Preparing merging #4544 in #4475 (level 3 of PR stack)

crusso and others added 30 commits May 17, 2024 16:28
Co-authored-by: Luc Blaeser <112870813+luc-blaeser@users.noreply.github.com>
@luc-blaeser luc-blaeser self-assigned this Jun 7, 2024
Copy link

github-actions bot commented Jun 10, 2024

Comparing from ae35f93 to bb46a86:
The produced WebAssembly code seems to be completely unchanged.

Comment on lines 53 to 55
debug_assert!(
tag == TAG_ARRAY_I || tag == TAG_ARRAY_M || tag == TAG_ARRAY_T || tag == TAG_ARRAY_S
);
Copy link
Contributor

@crusso crusso Jun 22, 2024

Choose a reason for hiding this comment

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

Suggested change
debug_assert!(
tag == TAG_ARRAY_I || tag == TAG_ARRAY_M || tag == TAG_ARRAY_T || tag == TAG_ARRAY_S
);
debug_assert!(is_base_array_tag(tag));

(Might need to import it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I addressed it in the lower PRs and merge the refactoring upwards.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM. There might be a bit of unnecessary overhead passing those mostly redundant object_kinds around but perhaps we shouldn't sweat the small stuff.

@luc-blaeser luc-blaeser marked this pull request as ready for review June 24, 2024 08:38
@luc-blaeser
Copy link
Contributor Author

LGTM. There might be a bit of unnecessary overhead passing those mostly redundant object_kinds around but perhaps we shouldn't sweat the small stuff.

Thank you very much for the review. Indeed, some object_kinds are redundant for specific cases in deserialize, but not for all cases. I guess this is the price of having a generic "mini-framework" for serialization/deserialization with a common trait for all the specific stable types.

@crusso
Copy link
Contributor

crusso commented Jun 24, 2024

(Still LGTM)

@luc-blaeser luc-blaeser merged commit bb46a86 into luc/graph-copy-on-stable-heap64 Jun 24, 2024
6 checks passed
@luc-blaeser luc-blaeser deleted the luc/graph-copy-on-stable-heap64-merge-object-tagging branch June 24, 2024 13:24
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