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.

Sign up for GitHub

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

Jump to bottom

Synchronization updates #416

Merged
merged 5 commits into from
Sep 16, 2015
Merged

Synchronization updates #416

merged 5 commits into from
Sep 16, 2015

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented Sep 7, 2015

Work in progress, todo:

  • update documentation
  • fix JRuby
  • tests
  • safe_initialization!
  • Java 7 support

@pitr-ch pitr-ch added the enhancement Adding features, adding tests, improving documentation. label Sep 7, 2015
@pitr-ch pitr-ch self-assigned this Sep 7, 2015
@pitr-ch pitr-ch added this to the 1.0.0 Release milestone Sep 7, 2015
@jdantonio
Copy link
Member

Looks great so far! Any idea when this will be ready to merge?

@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 9, 2015

Unless I run into issues, it should be merged in few days.

@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 11, 2015

I am doing one more biggish change around ensure_ivar_visibility!. It will be hidden behind a class inheritable flag safe_initialisation! Advantages are: easier for users to track, it's no longer in initialise; it only called once, no more duplications; it's called only when truly needed. I should have it done on weekend.

@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 12, 2015

Changes to discuss:

  • requires Fences of Java 8
  • will enforce @FinalIvar convention to make it testable that classes using it are marked with safe_initialization!

@jdantonio
Copy link
Member

We cannot require Java 8. As discussed here, JRuby 9000 supports both Java 7 and Java 8. That's why we are still using the internal implementation of ConcurrentHashMap (imported from thread_safe) rather than the Java 8 implementation.

@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 14, 2015

This is related to conversation in #357 too. I was remembering it wrong and I thought we are going to drop Java 8 support. I'll put the "best effort" hack for java 7 back and I'll keep working on this after rc.

@jdantonio sorry that it takes so long.

@jdantonio
Copy link
Member

At one point we talked about dropping Java 7 support but then Rails happened and @thedarkone reminded us of the need to support Java 7.

@pitr-ch
Introduce safe_initialization! class marker instead of ensure_ivar_vi…
a9da852 
…sibility!

* ensures full memory barier is inserted only when needed
* ensures that it's inserted only once
* testable correctness with @FinalIvar convention
@jdantonio
Copy link
Member

I've added the two tests that failed to my list of buggy tests. They aren't related to this PR.

@pitr-ch Is this ready to merge? The docs aren't necessary for a pre-release.

@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 16, 2015

Ok, I'll check the bug on travis, and I'll merge today.

@jdantonio
Copy link
Member

Cool. Once that's done I'll create the next pre-release and submit the thread_safe replacement PR to Rails. Thank you!

pitr-ch pushed a commit that referenced this pull request Sep 16, 2015
Merge pull request #416 from ruby-concurrency/synchronization
cd6a117 
Synchronization updates
@pitr-ch pitr-ch merged commit cd6a117 into master Sep 16, 2015
@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 16, 2015

@jdantonio Thanks for your patience.

@jdantonio
Copy link
Member

I've published a new pre-release and submit the PR to Rails which replaced thread_safe. That PR was merged. My priorities for 1.0 are interpreter warnings, buggy tests, and documentation. None of that will get in the way of your continued work on synchronization.

We're almost there! :-)

@pitr-ch
Copy link
Member Author

pitr-ch commented Sep 21, 2015

Thank you! I'll focus on the documentation for the layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pitr-ch pitr-ch

Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@pitr-ch @jdantonio