search

mmtk / ruby

Fork of The Ruby Programming Language [mirror], with added support for MMTk
https://www.ruby-lang.org/
Other
0 stars 1 forks source link

Merge branch 'master-as-of-9aa62bd' into mmtk #76

Closed eileencodes closed 6 days ago

eileencodes commented 2 months ago

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 https://github.com/mmtk/ruby/commit/9aa62bda46bf7f9de91a95c31fa09dafd23def37 Work in progress branch https://github.com/mmtk/ruby/compare/mmtk...Shopify:ruby:mmtk-with-master-gc-impl?expand=1

wks commented 2 months ago

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: https://github.com/mmtk/ruby/commit/1be8640395e8f3607152d3184352b3e40788a2d6), 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 commented 2 months ago

I think something very odd is going on with the mmtk fork. I noticed my change from https://github.com/mmtk/ruby/pull/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 commented 2 months ago

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 commented 2 months ago

@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 commented 6 days ago

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