search

vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 531 forks source link

Reduce heavy atomic operations on RoutingContextImplBase #2603

Open franz1981 opened 5 months ago

franz1981 commented 5 months ago

I see that 0cd7eca06ef17e09a8c87221d9b9f08d353614f7 from @tsegismont has introduced updaters, but I still don't understand why is using such heavy weight atomic operations there.

Given that is a pretty hot path for quarkus, it would be great to use different ones. Sending a PR soon, for evaluation

tsegismont commented 4 months ago

@franz1981 sorry for the delayed response. These atomic ops have been removed in https://github.com/vert-x3/vertx-web/pull/2545 and backported to 4.x in https://github.com/vert-x3/vertx-web/pull/2546

Vert.x 4.5.2 should be free of them.

franz1981 commented 4 weeks ago

I can still see them in https://github.com/vert-x3/vertx-web/blob/4.x/vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImplBase.java#L157-L158

These are using atomic volatile set(s) in the hotpaths and incrementAndGet which can cost hundreds of times more than normal non atomic operations: are really required? or they could be relaxed?

franz1981 commented 4 weeks ago

@tsegismont PTAL

tsegismont commented 4 weeks ago

Which PR?

franz1981 commented 4 weeks ago

There's no PR, but this comment at https://github.com/vert-x3/vertx-web/issues/2603#issuecomment-2332077259

In https://github.com/vert-x3/vertx-web/issues/2603#issuecomment-2122041437 it seems the atomics are not there anymore but the atomic operations are still there...

tsegismont commented 3 weeks ago

There's no PR, but this comment at #2603 (comment)

In #2603 (comment) it seems the atomics are not there anymore but the atomic operations are still there...

Thanks for the heads-up, yes I can see that now. I'll look into it