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

[Compatibility] Add Module#refinements #3093

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

itarato
Copy link
Collaborator

@itarato itarato commented Jun 5, 2023

Source: #3039

Module#refinements has been added. [Feature #12737]

@itarato itarato self-assigned this Jun 5, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 5, 2023
@itarato itarato added the shopify label Jun 5, 2023
@@ -1835,6 +1835,21 @@ def test_used_modules
assert_equal [ref::RefB, ref::RefA], ref::Combined::USED_MODS
end

def test_refinements
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eregon eregon self-requested a review June 6, 2023 11:55

@Specialization
protected RubyArray refinements(RubyModule self) {
return createArray(self.fields.getRefinements().values().toArray());
Copy link
Member

Choose a reason for hiding this comment

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

Needs a TruffleBoundary (on the specialization to keep it simple) for the .values() and .toArray()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

@itarato itarato force-pushed the feature/PA-add-refinement-class branch from 7fb546c to da66bcb Compare June 6, 2023 13:33
@itarato itarato requested a review from eregon June 6, 2023 16:07
@itarato itarato marked this pull request as ready for review June 6, 2023 16:07
@eregon eregon assigned eregon and unassigned itarato Jun 21, 2023
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Jun 21, 2023
@eregon eregon force-pushed the feature/PA-add-refinement-class branch from da66bcb to 1f012bb Compare June 21, 2023 14:48
@eregon
Copy link
Member

eregon commented Jun 22, 2023

An MRI test is failing (we should try to run those on GitHub Actions):

  1) Failure:
TestRefinement#test_refinements [/b/b/e/main/test/mri/tests/ruby/test_refinement.rb:1850]:
<[#<refinement:Integer@#<Module:0x115a3b8>>,
 #<refinement:String@#<Module:0x115a3b8>>]> expected but was
<[#<refinement:String@#<Module:0x115a3b8>>,
 #<refinement:Integer@#<Module:0x115a3b8>>]>.

@itarato itarato force-pushed the feature/PA-add-refinement-class branch from a6bc603 to 54d0c5d Compare June 23, 2023 16:16
@itarato
Copy link
Collaborator Author

itarato commented Jun 23, 2023

An MRI test is failing (we should try to run those on GitHub Actions):

  1) Failure:
TestRefinement#test_refinements [/b/b/e/main/test/mri/tests/ruby/test_refinement.rb:1850]:
<[#<refinement:Integer@#<Module:0x115a3b8>>,
 #<refinement:String@#<Module:0x115a3b8>>]> expected but was
<[#<refinement:String@#<Module:0x115a3b8>>,
 #<refinement:Integer@#<Module:0x115a3b8>>]>.

@eregon To respect ordering I switched to linked hash map, which has I believe has a minor disadvantage on create (in time) as well as I believe using atomicity via Collections.synchronizedMap is slighly slower than ConcurrentHashMap. Would we consider this acceptable or should we look for alternatives?

@itarato itarato requested a review from eregon June 23, 2023 16:23
@itarato itarato removed the in-ci The PR is being tested in CI. Do not push new commits. label Jun 23, 2023
@eregon
Copy link
Member

eregon commented Jun 26, 2023

There are two problems with that:

  • Collections.synchronizedMap() does not allow parallel lookups. And so it means method lookup in different threads would cause contention if there are refinements active in the scope where the lookup happens.
  • Collections.synchronizedMap() does not synchronize iterables like for (var entry : map.entrySet()), which is then incorrect synchronization and causes all sorts of problems.

I think the test should allow both, ordering there seems artificial and unnecessary, it will not affect method lookup semantics, as anyway it's a single refinement module per class/module being refined inside a namespace module.
Could you either tag the MRI test or not add it in this PR? There is already a spec doing the appropriate check.

@itarato itarato force-pushed the feature/PA-add-refinement-class branch from 54d0c5d to 3965709 Compare June 26, 2023 16:00
@itarato
Copy link
Collaborator Author

itarato commented Jun 26, 2023

There are two problems with that:

* `Collections.synchronizedMap()` does not allow parallel lookups. And so it means method lookup in different threads would cause contention if there are refinements active in the scope where the lookup happens.

* `Collections.synchronizedMap()` does not synchronize iterables like `for (var entry : map.entrySet())`, which is then incorrect synchronization and causes all sorts of problems.

I think the test should allow both, ordering there seems artificial and unnecessary, it will not affect method lookup semantics, as anyway it's a single refinement module per class/module being refined inside a namespace module. Could you either tag the MRI test or not add it in this PR? There is already a spec doing the appropriate check.

@eregon Removed that change and tagged the MRI test.

@itarato itarato force-pushed the feature/PA-add-refinement-class branch from 3965709 to af5b297 Compare June 26, 2023 16:01
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Jun 28, 2023
@graalvmbot graalvmbot merged commit 555d379 into oracle:master Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants