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

Improve attribute handling in RequestPredicates #30028

Closed

Conversation

yuzawa-san
Copy link
Contributor

@yuzawa-san yuzawa-san commented Feb 24, 2023

I was doing some memory allocation flame chart analysis and found that there was a lot of copying of the attributes map in the AND/OR/NOT predicates which I believe can be avoided. The intent appeared to be to support the PathPatternPredicate which is the only implementation that updates the request state. Given that the PathPatternPredicate can be embedded within any number of boolean operators the save, evaluate, and restore operation was added in those predicates.

The solution I had was to introduce some additional classes which allow for the predicate to make changes. This is the Evaluation record which has the test result and whatever state changes should be done if the caller deems it so.

Open question: the PathPatternPredicate appeared to be the only implementation which updated the request, but do folks have state-modifying predicates out in the wild? It would appears physically possible, but at the same time against the @FunctionalInterface nature of the definition of RequestPredicate. So this could break extant use cases, but those use cases are not necessarily philosophically correct. Given PathPatternPredicate is internal (and "rule breaking" by changing state), I supposed internally we could support its rule breaking.

The existing RequestPredicateAttributesTests all pass still.

I only did the changes in webflux, but could port these changes over to the servlet functions as well later if this is accepted.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 24, 2023
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Feb 24, 2023
@sdeleuze sdeleuze requested a review from poutsma March 4, 2023 15:42
@poutsma poutsma self-assigned this Mar 7, 2023
@poutsma
Copy link
Contributor

poutsma commented Mar 9, 2023

Interesting. I will take a closer look for 6.1 M1.

@poutsma poutsma added this to the 6.1.0-M1 milestone Mar 9, 2023
@sbrannen sbrannen removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 9, 2023
@yuzawa-san
Copy link
Contributor Author

@poutsma Thanks, here is some background. I have a few dozen routes registered like so:

RouterFunctions.Builder builder = RouterFunctions.route();
builder.add(RouterFunctions.route(RequestPredicates.GET("/foo"), fooHandler));
builder.add(RouterFunctions.route(RequestPredicates.GET("/bar"), barHandler));
....
return builder.build();

and I collected this memory icicle graph:
image

and this cpu icicle graph:
image

The saving and restoring uses the majority of the memory and cpu relative to the actual evaluation of the HttpMethodPredicate and PathPatternPredicate

My handlers dont use much cpu or memory, so this was constituting an non-negligible percentage of the applications overall footprint.

@yuzawa-san
Copy link
Contributor Author

Here are some more extensive icicle graphs generated from my stress test.

memory before (the selected item is basically all of the "memory after" graph):
image

memory after:
image

cpu before:
image

cpu after:
image

and i have two more optimizations within that which should cut down even more: #30138 and #30139

@poutsma
Copy link
Contributor

poutsma commented Mar 20, 2023

@yuzawa-san Great work, thanks!

There was a lot of copying of the attributes map in the AND/OR/NOT predicates which I believe can be avoided.
@poutsma poutsma modified the milestones: 6.1.0-M1, 6.1.0-M2 Jun 13, 2023
@poutsma poutsma modified the milestones: 6.1.0-M2, 6.1.0-M3 Jul 10, 2023
@jhoeller jhoeller modified the milestones: 6.1.0-M3, 6.1.0-M4 Jul 17, 2023
@poutsma poutsma modified the milestones: 6.1.0-M4, 6.1.0-RC1 Aug 15, 2023
@poutsma poutsma removed their request for review September 12, 2023 09:59
@poutsma
Copy link
Contributor

poutsma commented Sep 12, 2023

This PR has now been merged, though I did some refactoring and renaming.

Thank you for your contribution!

@poutsma poutsma closed this in 39786e4 Sep 12, 2023
poutsma added a commit that referenced this pull request Sep 12, 2023
* gh-30028:
  Polishing external contribution
  Improve attribute handling in RequestPredicates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants