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 branch 'master-as-of-9aa62bd' into mmtk #76

Closed
wants to merge 1 commit into from

Conversation

eileencodes
Copy link
Collaborator

We're working on merging master into mmtk and fix the conflicts now that gc.c has been split into gc.c and gc_impl.c. I'm sending this PR so that we can have a smaller diff. This merges (and fixed conflicts) for all commits before the gc api was merged.

Merges up to 9aa62bd
Work in progress branch https://github.com/mmtk/ruby/compare/mmtk...Shopify:ruby:mmtk-with-master-gc-impl?expand=1

@wks
Copy link

wks commented Jul 15, 2024

Strangely, the branch eileencodes:master-as-of-9aa62bd is just one single commit after the mmtk branch, and is not a descendant of the actual commit 9aa62bd. I merged the revision 9aa62bd with the mmtk branch manually (See: 1be8640), and the resulting tree is identical to eileencodes:master-as-of-9aa62bd.

I'll prioritize merging the master branch and the mmtk branch. I'll have a look at your work-in-progress branch first.

@eileencodes
Copy link
Collaborator Author

I think something very odd is going on with the mmtk fork. I noticed my change from #74 isn't in the mmtk branch on Friday and I can't figure out why. In addition the code that it changed is missing as well. Did a branch maybe get force pushed somehow and delete those commits?

@wks
Copy link

wks commented Jul 15, 2024

I think something very odd is going on with the mmtk fork. I noticed my change from #74 isn't in the mmtk branch on Friday and I can't figure out why. In addition the code that it changed is missing as well. Did a branch maybe get force pushed somehow and delete those commits?

Yes. #74 fixed a bug in my previous commit that optimized the handling of "PPPs", but also revealed other bugs in it and made subsequent PRs fail in the CI. Therefore, I reverted those commits about PPPs, including the change you introduced. I briefly mentioned it here, but forgot to tell you about that. Sorry about the confusion.

@wks
Copy link

wks commented Jul 17, 2024

@eileencodes I made a few changes over your work-in-progress branch. As a result, it is able to finish all currently enabled btest and test-all cases, and is able to run the Liquid benchmark. Here is the diff: https://github.com/Shopify/ruby/compare/mmtk-with-master-gc-impl-rebase...wks:ruby:mmtk-with-master-gc-impl-rebase?expand=1

Among the changes I made, I reverted two commits related to the "PPP" which I pushed to the mmtk branch and then forcefully removed from the mmtk branch. (Sorry about the confusion.) With the two commits reverted, the code will link properly with the current master branch of mmtk-ruby (https://github.com/mmtk/mmtk-ruby). Otherwise, the ruby repo and the mmtk-ruby repo will disagree about the fields in the MMTk_RubyUpcalls struct.

The other two changes are about obj_type_name and RVALUE_WB_UNPROTECTED.

I simply overrode RVALUE_WB_UNPROTECTED in default.c with MMTk-specific implementation, like in the mmtk branch.

obj_type_name is a bit complicated. It is defined in gc.c, but is used by some RUBY_DEBUG_LOG statements in default.c. In theory, there should be a compilation error. But in the master branch, RUBY_DEBUG_LOG is defined as a no-op in default.c even though I set the -DUSE_RUBY_DEBUG_LOG=1 compiler flag. But in https://github.com/Shopify/ruby/tree/mmtk-with-master-gc-impl-rebase, the RUBY_DEBUG_LOG macro is effective, and there will be compilation error if I set -DUSE_RUBY_DEBUG_LOG=1. I think it may be related to the header files included.

@wks
Copy link

wks commented Sep 9, 2024

We have synchronized our development branches with the upstream master in other PRs. We can close this now.

@wks wks closed this Sep 9, 2024
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