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

Additional type pollution fix #224

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

Sanne
Copy link
Contributor

@Sanne Sanne commented May 23, 2023

Similar to #190 , this looks rather trivial but has severe performance implications.

I wonder if we shouldn't delete all uses of Assert.checkNotNullParam, it seems rather dangerous.

@franz1981
Copy link
Contributor

Avoiding the cast by returning void will work too

@Sanne
Copy link
Contributor Author

Sanne commented May 24, 2023

Avoiding the cast by returning void will work too

right that's essentially what I've done here: ignore the return type. Perhaps that would be a good change to the Assert.checkNotNullParam so to avoid the problem in future.. WDYT @radcortez ?

@radcortez
Copy link
Member

right that's essentially what I've done here: ignore the return type. Perhaps that would be a good change to the Assert.checkNotNullParam so to avoid the problem in future.. WDYT @radcortez ?

Sure. We probably need to deprecate that method and introduce a new one.

@Ladicek
Copy link
Contributor

Ladicek commented May 24, 2023

Something like this.foo = Objects.requireNonNull(foo) (or Assert.checkNotNullParam() for that matter) is a very common idiom in constructors. I have no idea why it was used here (and I'm obviously +1 for avoiding it), but removing the Assert.checkNotNullParam() method is not gonna remove the usages of this idiom. The Objects.requireNonNull() method exists in the JDK, and I'm pretty sure a lot of other projects have their own copy as well (I know I do in SmallRye Fault Tolerance, for example).

@dmlloyd
Copy link
Collaborator

dmlloyd commented May 24, 2023

I have to also add a 🛑 to this line of reasoning... what we're working around here is a JVM bug which people are working on fixing, whereas an API change is much more long-term. As @Ladicek pointed out this is a commonly used pattern so changing the API isn't really warranted.

I'm OK with some simple/localized band-aid type fixes for the type pollution issue to stop the bleeding but I am against significant reengineering or API redesigning to work around it. Once the issue is fixed, I feel we will regret the amount of effort we spent on the problem which could have been spent in other areas, and we will also regret how we hobbled our own programming models for a short-term gain.

@Sanne
Copy link
Contributor Author

Sanne commented Sep 26, 2023

Could this get merged?

@dmlloyd dmlloyd merged commit 30094c3 into smallrye:main Sep 26, 2023
5 checks passed
@Sanne Sanne deleted the AvoidTypeCachePollutions2 branch September 26, 2023 14:08
@Sanne
Copy link
Contributor Author

Sanne commented Sep 26, 2023

Thanks!

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.

None yet

5 participants