Skip to content

Optimize Request Predicate attribute merging #32245

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
Closed
poutsma opened this issue Feb 12, 2024 · 3 comments
Closed

Optimize Request Predicate attribute merging #32245

poutsma opened this issue Feb 12, 2024 · 3 comments
Assignees
@poutsma
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone
6.2.0-M1

Comments

@poutsma
Copy link
Contributor

poutsma commented Feb 12, 2024

When using router functions, the bulk of RequestPredicate execution is spent in attribute map merging. We should consider ways to improve this, for instance by using composite maps instead of allocating new ones.
@poutsma poutsma added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Feb 12, 2024
@poutsma poutsma added this to the 6.2.x milestone Feb 12, 2024
@poutsma poutsma self-assigned this Feb 12, 2024
@injae-kim
Copy link
Contributor

injae-kim commented Feb 14, 2024 •
edited by sbrannen
Loading

spring-framework/spring-webmvc/src/main/java/org/springframework/web/servlet/function/RouterFunctions.java

Line 1247 in 6e9607c

return Collections.unmodifiableMap(new LinkedHashMap<>(attributes));

spring-framework/spring-webmvc/src/main/java/org/springframework/web/servlet/function/RouterFunctions.java

Lines 1267 to 1269 in 6e9607c

Map<String, Object> attributes = new LinkedHashMap<>(this.attributes);
attributes.put(name, value);
return new AttributesRouterFunction<>(this.delegate, attributes);

What an interesting issue! I think we can use some CompositeMap instead of UnmodifiableMap & deep copy on every withAttributes() on AttributesRouterFunction.
(I'm not sure above is what this issue wants)

I fixed similar issue on another opensource before, so if you busy I can create PR :) thank you!

@poutsma
Copy link
Contributor Author

poutsma commented Feb 14, 2024

@injae-kim There are a lot of intrinsic details that can go wrong here, which has caused many regressions in the past. So I'd rather tackle this one myself, thanks!

@injae-kim
Copy link
Contributor

injae-kim commented Feb 14, 2024 •
edited
Loading

which has caused many regressions in the past.

Aha thanks for the explanation! I got it.
Please share on this issue if there's something I can help 😃

@kilink kilink mentioned this issue Feb 18, 2024
Closed
@poutsma poutsma modified the milestones: 6.2.x, 6.2.0-M1 Feb 22, 2024
poutsma added a commit that referenced this issue May 2, 2024
@poutsma
Ensure old attributes are not removed by accident
d5664ba 
This commit ensures that a copy is made of old attributes before
replacing it with new attributes. Because new attributes can be
composed of old, clearing the old would also remove entries from the
 new.

See gh-32245
poutsma added a commit that referenced this issue May 2, 2024
@poutsma
Resolve collisions in composite collections
3897ea7 
Before this commit, creating a CompositeMap from two maps with the same
key has strange results, such as entrySet returning duplicate entries
with the same key.

After this commit, we give precedence to the first map by filtering out
all entries in the second map that are also mapped by the first map.

See gh-32245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@poutsma poutsma

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Milestone
6.2.0-M1
Development

No branches or pull requests
2 participants
@poutsma @injae-kim