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

Flux.index operator calls toString() on every element passing through the operator #1453

Closed
devstdout opened this issue Nov 30, 2018 · 3 comments
Closed

Flux.index operator calls toString() on every element passing through the operator #1453

devstdout opened this issue Nov 30, 2018 · 3 comments
Labels
good first issue Ideal for a new contributor, we'll help type/enhancement A general enhancement
Milestone
3.2.4.RELEASE

Comments

@devstdout
Copy link

FluxIndex.onNext(T element) inadvertently calls element.toString() in order to create sensible error message when calling Objects.requireNonNull. This can cause performance issue if element.toString() is an expensive operation.

Defer the creation of the error message when it is really needed (ie when indexMapper.apply() returns null).

This applies to FluxIndexFusable as well.

Reactor Core version

3.2.3
@simonbasle simonbasle added type/enhancement A general enhancement good first issue Ideal for a new contributor, we'll help labels Nov 30, 2018
@simonbasle simonbasle added this to the Backlog milestone Nov 30, 2018
@simonbasle
Copy link
Member

Good point, and it is simple enough to fix by using the Supplier<String> variant of requireNonNull.

@npathai
Copy link
Contributor

npathai commented Dec 3, 2018

@simonbasle I would like to work on this

@simonbasle
Copy link
Member

@npathai sure, go ahead! Be sure to read CONTRIBUTING.md first, but that should be quite a simple change.

npathai added a commit to npathai/reactor-core that referenced this issue Dec 4, 2018
@npathai
fix reactor#1453 Converted eager toString call from index operator to…
7cce1da 
… lazy

Due to use of Objects.requireNonNull(Object, String) varaiant, toString for each element that was passing through the operator was evaluated eagerly. To change that used Objects.requireNonNull(Object, Supplier<String>) varaiant which makes toString call lazy. Only if indexMapper.apply() will return null, then the toString will be called.
npathai added a commit to npathai/reactor-core that referenced this issue Dec 4, 2018
@npathai
fix reactor#1453 Converted eager toString call from index operator to…
8558c0d 
… lazy

Rather than using Supplier<String> variant of Objects.requireNonNull() which would have created Supplier instances unnecessarily, used traditional null check. As the check for null safe index mapping was repeated thrice in FluxIndex, created a NullSafeIndexMapper class which wraps actual indexMapper and provides null safe check. NullSafeIndexMapper instance is created once from the constructor.
@simonbasle simonbasle modified the milestones: Backlog, 3.2.4.RELEASE Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Ideal for a new contributor, we'll help type/enhancement A general enhancement
Projects
None yet
Milestone
3.2.4.RELEASE
Development

No branches or pull requests
3 participants
@npathai @simonbasle @devstdout