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

WFCORE-6863 Add ServiceDescriptor convenience method for adding a requirement to a RuntimeCapability. #6046

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

pferraro
Copy link
Contributor

https://issues.redhat.com/browse/WFCORE-6863

Reduce heap requirements (by eliminating hash table allocation) for most use cases of addRequirements(...).

…uirement to a RuntimeCapability.

Reduce heap requirements (by eliminating hash table allocation) for most use cases of addRequirements(...).
@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jun 22, 2024
@wildfly-ci

This comment was marked as off-topic.

@wildfly-ci

This comment was marked as off-topic.

return this.addRequirements(requirements[0].getName(), requirements[1].getName());
default:
Stream<String> mapped = Stream.of(requirements).map(NullaryServiceDescriptor::getName);
this.requirements = (this.requirements.isEmpty() ? mapped : Stream.concat(this.requirements.stream(), mapped)).collect(Collectors.toUnmodifiableSet());
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this.requirements.isEmpty() is true, this.requirements should be assigned to an unmodificable Set, however Stream<String> mapped is not an unmodificable set at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, out of curiosity, why not use the code for the default: case always and remove the need for a switch? I mean, why use case 1 and 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this.requirements.isEmpty() is true, this.requirements should be assigned to an unmodificable Set, however Stream mapped is not an unmodificable set at this point.

I'm not sure what you mean. Both cases use the same collector that returns an unmodifiable set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, out of curiosity, why not use the code for the default: case always and remove the need for a switch? I mean, why use case 1 and 2?

Because we want to avoid allocating a hash table for only 1 or 2 entries, if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pferraro

I'm not sure what you mean. Both cases use the same collector that returns an unmodifiable set.

Forget it, I see it now, sorry for the noise about that.

return this.addRequirements(requirements[0].getName(), requirements[1].getName());
default:
Stream<String> mapped = Stream.of(requirements).map(NullaryServiceDescriptor::getName);
this.requirements = (this.requirements.isEmpty() ? mapped : Stream.concat(this.requirements.stream(), mapped)).collect(Collectors.toUnmodifiableSet());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pferraro

I'm not sure what you mean. Both cases use the same collector that returns an unmodifiable set.

Forget it, I see it now, sorry for the noise about that.

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Jun 25, 2024
@yersan yersan merged commit b9ffd51 into wildfly:main Jun 25, 2024
12 checks passed
@yersan
Copy link
Collaborator

yersan commented Jun 25, 2024

Thanks @pferraro

@pferraro pferraro deleted the WFCORE-6863 branch July 24, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants