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

Remove -Xcheckinit compiler flag #2540

Merged
merged 1 commit into from
Nov 14, 2021

Conversation

armanbilge
Copy link
Member

I'm pretty sure this is responsible for those auto-generated volatile fields that have been taunting @vasilmkd 😆

@armanbilge armanbilge changed the title Remove -Xcheck-init compiler flag Remove -Xcheckinit compiler flag Nov 14, 2021
@vasilmkd vasilmkd requested a review from djspiewak November 14, 2021 00:54
@vasilmkd
Copy link
Member

vasilmkd commented Nov 14, 2021

I'm in favor of removing this, because it causes a volatile field bitmap$init to be created on a lot of classes. This slows down constructors and field accesses (which we work hard to avoid), but the constructors remain. Examples:
#2534 (comment) and #2473.

@armanbilge
Copy link
Member Author

I don't know how hard it would be to setup, but if this check is important, maybe we can enable this flag only for testing/CI but remove for release?

@vasilmkd
Copy link
Member

No, I don't agree with that. That could hide bugs. Either there is a good reason that this flag was included and @djspiewak can shed some light on it, or I really don't know why this is here and we can remove it everywhere.

@armanbilge
Copy link
Member Author

@vasilmkd
Copy link
Member

Before:

[info] Benchmark             (size)   Mode  Cnt      Score     Error  Units
[info] AsyncBenchmark.start     100  thrpt    5  12890.043 ± 107.889  ops/s

After:

[info] Benchmark             (size)   Mode  Cnt      Score     Error  Units
[info] AsyncBenchmark.start     100  thrpt    5  15683.946 ± 296.763  ops/s

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Sincerely no idea why I explicitly added this. So odd.

@djspiewak djspiewak merged commit b28dd40 into typelevel:series/3.x Nov 14, 2021
@armanbilge
Copy link
Member Author

@vasilmkd should we consider reverting some of the changes you made to workaround this? E.g. moving things into constructors. Or maybe those were good ideas anyway 👍

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.

3 participants