Skip to content

fix #1962 Avoid extending Entry in Context1#1963

Merged
simonbasle merged 2 commits into
masterfrom
1962-avoidContext1Entry
Nov 22, 2019
Merged

fix #1962 Avoid extending Entry in Context1#1963
simonbasle merged 2 commits into
masterfrom
1962-avoidContext1Entry

Conversation

@simonbasle
Copy link
Copy Markdown
Contributor

@simonbasle simonbasle commented Nov 20, 2019

The motivation for this change is that this drives two implementations
of Context#stream() to use the Context1 as an immutable map entry.

In turn, this proves problematic when the stream() is converted to a
string representation (eg. by loggers or assertions): the stream entries
use Context1{k=v} format, which is fine in a standalone toString but
less so in a representation of the whole stream.

AbstractMap.SimpleImmutableEntry can be leveraged instead, and it
has a proper key=value string representation by default.

@simonbasle simonbasle added this to the 3.3.1.RELEASE milestone Nov 20, 2019
@simonbasle simonbasle self-assigned this Nov 20, 2019
@bsideup
Copy link
Copy Markdown
Contributor

bsideup commented Nov 20, 2019

@simonbasle Context.putAll and ContextN.putAll use other.stream() which will create quite some allocations. Perhaps we should override them where possible and use the internal state

@simonbasle
Copy link
Copy Markdown
Contributor Author

simonbasle commented Nov 20, 2019

@bsideup default putAll is a bit wasteful for Context0-Context5, true: it invokes put multiple times (no real way around that if we want to check for duplicate keys and all) but also calls stream(), which unnecessarily creates map entries. This can be fixed by each specific implementation.

The default method should stay for compatibility, right? (since someone out there could have implemented Context without putAll)

I don't think the ContextN implementation can really be improved: it avoids intermediate new ContextX by preparing an internal map and feeding it both the current internal map and the other context as a map if it is not a ContextN itself. That last part limits the damages, because it implies there will be at most 5 "garbage" entries instantiated when creating the "other" map 👍

@simonbasle
Copy link
Copy Markdown
Contributor Author

actually @bsideup there is no real way around the stream conversion, because the operation is not symmetric. The other values must be merged into the current context, potentially "replacing" existing keys. So we need some way of iterating, and the only one in the public API is stream()...

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 20, 2019

Codecov Report

Merging #1963 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1963      +/-   ##
============================================
+ Coverage     81.64%   81.66%   +0.02%     
+ Complexity     3982     3980       -2     
============================================
  Files           374      374              
  Lines         30811    30807       -4     
  Branches       5798     5798              
============================================
+ Hits          25156    25160       +4     
+ Misses         4060     4054       -6     
+ Partials       1595     1593       -2
Impacted Files Coverage Δ Complexity Δ
...e/src/main/java/reactor/util/context/Context1.java 100% <100%> (ø) 11 <1> (-3) ⬇️
...e/src/main/java/reactor/util/context/ContextN.java 100% <100%> (ø) 20 <1> (-1) ⬇️
.../java/reactor/core/scheduler/ElasticScheduler.java 84.07% <0%> (-1.77%) 26% <0%> (ø)
...r-core/src/main/java/reactor/core/Disposables.java 88.95% <0%> (-1.23%) 21% <0%> (ø)
.../java/reactor/core/publisher/BlockingIterable.java 78.12% <0%> (-1.05%) 7% <0%> (ø)
...ain/java/reactor/core/publisher/FluxPublishOn.java 86.57% <0%> (+0.2%) 6% <0%> (ø) ⬇️
...c/main/java/reactor/core/publisher/FluxExpand.java 95.51% <0%> (+0.44%) 3% <0%> (ø) ⬇️
...in/java/reactor/core/publisher/FluxWindowWhen.java 84.13% <0%> (+2.4%) 2% <0%> (ø) ⬇️
...va/reactor/core/publisher/ParallelMergeReduce.java 72.95% <0%> (+3.27%) 3% <0%> (ø) ⬇️
...ain/java/reactor/core/scheduler/SchedulerTask.java 76.78% <0%> (+3.57%) 17% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7b11d8...4541db3. Read the comment docs.

@simonbasle
Copy link
Copy Markdown
Contributor Author

@bsideup created #1964 to address your comment on limiting garbage generation. This is superseded by #1965

@simonbasle simonbasle added status/duplicate This is a duplicate of another issue status/on-hold This was set aside or cannot be worked on yet labels Nov 20, 2019
The motivation for this change is that this drives two implementations
of `Context#stream()` to use the `Context1` as an immutable map entry.

In turn, this proves problematic when the `stream()` is converted to a
string representation (eg. by loggers or assertions): the stream entries
use `Context1{k=v}` format, which is fine in a standalone toString but
less so in a representation of the whole stream.

AbstractMap.SimpleImmutableEntry can be leveraged instead, and has a
proper `key=value` string representation by default.

Also added tests around putAll-replaces case.
@simonbasle simonbasle force-pushed the 1962-avoidContext1Entry branch from 63d788b to 3821d25 Compare November 22, 2019 09:16
@simonbasle simonbasle removed status/duplicate This is a duplicate of another issue status/on-hold This was set aside or cannot be worked on yet labels Nov 22, 2019
@simonbasle
Copy link
Copy Markdown
Contributor Author

since there is a doubt about #1964, this PR is now no more a duplicate.

@simonbasle simonbasle force-pushed the 1962-avoidContext1Entry branch from 3821d25 to 4541db3 Compare November 22, 2019 10:11
@simonbasle simonbasle merged commit d8ca137 into master Nov 22, 2019
@simonbasle simonbasle deleted the 1962-avoidContext1Entry branch November 22, 2019 15:51
@simonbasle simonbasle removed this from the 3.3.1.RELEASE milestone Nov 22, 2019
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.

4 participants