-
Notifications
You must be signed in to change notification settings - Fork 38.3k
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
Optimize some iterations in BodyExtractor and BodyInserter #30136
Conversation
they use less memory and cpu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed these kind of stream usage can get an order of magnitude slower especially for small collections... this looks good to me overall, thanks for the PR @yuzawa-san 👍
one minor comment to address from a readability standpoint and I think we can merge this in time for 6.0.8
.orElseGet(() -> Mono.error(unsupportedError(bodyType, context, mediaType))); | ||
for (HttpMessageWriter<?> messageWriter : context.messageWriters()) { | ||
if (messageWriter.canWrite(bodyType, mediaType)) { | ||
return write(publisher, bodyType, mediaType, outputMessage, context, cast(messageWriter)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compared to the BodyExtractor
case, I find that cast
call is a little too buried among all these parameters, making it less visible. would you mind extracting it to an intermediate local variable? this should be efficiently optimized away by the JIT compiler, so that is purely from a readability perspective.
use an intermediate variable for readability
@simonbasle updated with intermediate variable. #29972 also gets around another (I would say more expensive) streams usage in ReadOnlyHttpHeaders. spring-framework/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java Lines 150 to 156 in 4e896c8
I left this one there because that PR avoids that entrySet and also I have been working on a branch where I got ReadOnlyHttpHeaders to use CollectionUtils.unmodifiableMultiValueMap internally.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the change @yuzawa-san 👍
Using iterators for trivial findFirst and toList cases uses less CPU and memory (in these cases, not while costing readability)
Here is an icicle graph example of the CPU usage before
and here it is after:
There was about a 10-15% overhead with the streams usage, which we can save on by doing this.