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

PathMappings optimizations #9055

Merged
merged 7 commits into from Dec 20, 2022
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 14, 2022

Avoid iterations if only ServletPathSpec instances
Avoid tests for empty mappings.
Better reset implementation

Avoid iterations if only ServletPathSpec instances
Avoid tests for empty mappings.
Better reset implementation
Avoid iterations if only ServletPathSpec instances
Avoid tests for empty mappings.
Better reset implementation
@gregw
Copy link
Contributor Author

gregw commented Dec 14, 2022

This is a more comprehensive alternative to #9053

@gregw gregw requested a review from lorban December 15, 2022 12:41
@joakime joakime added this to the 10.0.x milestone Dec 15, 2022
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs more tests

Improve suffix matching
@gregw gregw requested a review from joakime December 16, 2022 01:02
Improve exact matching
@lorban
Copy link
Contributor

lorban commented Dec 16, 2022

It feels like a set of micro benchmarks would help now that this class' perf appeared on the radar.

@gregw
Copy link
Contributor Author

gregw commented Dec 16, 2022

It feels like a set of micro benchmarks would help now that this class' perf appeared on the radar.

Maybe, but these changes are primarily removing work that was never actually necessary, so it is more of a bug fix than an optimization. I don't think we should hold up a merge of this whilst over analyzing it. I'd like to get this merged and only if we then see it on flame graphs should we drill down with micro benchmarks.

@gregw gregw requested a review from janbartel December 19, 2022 10:37
joakime
joakime previously approved these changes Dec 19, 2022
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.
Should probably merge PR #9071 first (or revert your IncludeExcludeSet change in this PR)

@gregw gregw merged commit 4916377 into jetty-10.0.x Dec 20, 2022
@gregw gregw deleted the jetty-10.0.x-PathMapping-redux branch December 20, 2022 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants