Adding new methods to manage the path params map #2645
Conversation
franz1981
force-pushed
the
4.x_path_params
branch
from
fca143e to
28d5a15
Compare
franz1981
changed the title
|
PTAL @vietj as well |
| * Add a new one or replace an existing path parameter | ||
| * @throws NullPointerException when the name or value is null | ||
| */ | ||
| void addOrReplacePathParam(String name, String value); |
There was a problem hiding this comment.
Do we really need this new method? It will create the underlying map if needed anyway.
There was a problem hiding this comment.
good point; it was consistent to not expose the underlying form/nature/content of the map - in order to enable to not allocate it, if possible
But any suggestion is more than welcome, bud 🙏 and thanks for looking at this!
| boolean removePathParam(String s); | ||
|
|
||
| /** | ||
| * Returns an unmodifiable map of named parameters as defined in path declaration with their actual values |
There was a problem hiding this comment.
This is a breaking change that is not mandatory (even if I understand the intention). Can you please revert it?
There was a problem hiding this comment.
Do you mean that you prefer to expose it directly as it was? without being not modifiable?
There was a problem hiding this comment.
Yes, because we some users will expect this to be modifiable.
| if (pathParams == null) { | ||
| pathParams = new HashMap<>(); | ||
| // let's start small | ||
| pathParams = new HashMap<>(1); |
There was a problem hiding this comment.
Isn't this too aggressive? How about using 4, which allows to store 3 path params without rehashing?
There was a problem hiding this comment.
in theory with 1 it should have 2 as capacity - but yes, anything but the default can be a good improvement; given that it happen in our hot path, saving half array is still wellcome!
There was a problem hiding this comment.
Then, let's be a little conservative and go with 4.
Co-authored-by: Thomas Segismont <[email protected]>
|
The failure looked unrelated, I've restarted the job |
I've fallen into this while working on improving allocations for Quarkus:
vertx-web/vertx-web/src/main/java/io/vertx/ext/web/impl/RouteState.java
Lines 1225 to 1230 in ed41309
pathParams, while stillnullcause allocating itAnd, at the same time, most of the time, the path params are just 1 or 2; meaning that the allocated map doesn't need to have an initial capacity that big.