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

feautre: Relaxed memory model for final fields, strict semantics with @safePublish annotation #3719

Merged

Conversation

WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented Jan 30, 2024

resolves #3673
Final fields are now by default unsynchronized. We introduce a @safePublish annotation to restore Java final fields semantics

TODO:

  • - Introduce a compile time config (not linking time) that library authors can use to automatically apply the "@safePublish" annotation to all vals within an entire compilation unit. This would essentially be an escape hatch.
  • - Introduce a linking time config that applications can use to restore final Field Semantics for all vals, in their own code and library NIR. Another escape hatch.

…efault don't synchronize final fields. Mark most of java.util.concurrent types with new annotation
@armanbilge armanbilge self-requested a review January 30, 2024 02:37
Comment on lines 606 to 610
// It's required only on the first access to obj fields
if (field.attrs.finalWithSafePublish && acquiredFinalFieldsOf.get.add(
obj
))
buf.fence(nir.MemoryOrder.Acquire)
Copy link
Member

Choose a reason for hiding this comment

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

What is this doing exactly? It's required on the first access per thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a very simple optimisation, for cases when we want to load multiple fields from the same object. This way we have at most 1 fence per object within a single method. Previously we're introducing a fence for each load, although now as I'm thinking about it it's bugged and should be reverted - we might add fence in only one conditional branch of code, leaving other one vulnerable.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, good catch.

I think we should always emit NIR with all the barriers. Then we can implement an NIR => NIR optimization phase that attempts to "coalesce" multiple barriers. The JVM implements these sorts of optimizations.

I am trying to find more information about this, but here is an example:
https://bugs.openjdk.org/browse/JDK-8032218

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking about this, and also with coalescing of synchronized blocks. With these it should be easier, although probably we should create a dedicated NIR instruction for them to make it easier. Moving generation of fences from lowering to NIR generation would be required to correctly optimize them, but it would also harder to identify possibly redundant fences when using relaxed memory model - we'd need to perform a look ahead to check that. Anyway that's one important change to do in the future

@WojciechMazur
Copy link
Contributor Author

@armanbilge I'm going to merge it into the main before the end of your review to unblock some of other work, however fell free to comment here, and I will definitely address any found issues.

@WojciechMazur WojciechMazur merged commit 5bfb73a into scala-native:main Jan 30, 2024
58 of 62 checks passed
@WojciechMazur WojciechMazur deleted the feautre/relaxed-memory-model branch January 30, 2024 13:03
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.

Consider having a configurable memory model
2 participants