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

latin1StringToBytes: use standard method #4980

Closed
wants to merge 10 commits into from

Conversation

magicprinc
Copy link
Contributor

@magicprinc magicprinc commented Nov 28, 2023

  1. String#getBytes(StandardCharsets.ISO_8859_1) is ~4% faster than manual copying in a for loop
  2. and one line of Java code instead of 6

➕ missing @Override

String#getBytes(StandardCharsets.ISO_8859_1) is ~4% faster than manual copying in loop
and one line of java code instead of 6
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Can you add the benchmark which shows this is better than the for loop?

import java.util.Objects;

/**
* @author <a href="http://tfox.org">Tim Fox</a>
* @author <a href="mailto:[email protected]">Lars Timm</a>
*/
public class RecordParserImpl implements RecordParser {
public final class RecordParserImpl implements RecordParser {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't change this, it won't change much and is unrelated to the subject of the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDEA shows me a lot of recommendations and best practices 😥

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, with Intellij's default, you get thousands of suggestions. It's not wrong to use them, but if you're sending a PR for a specific topic, please focus on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!✅🤝

@vietj
Copy link
Member

vietj commented Dec 1, 2023

@magicprinc you should avoid doing too many changes in your pull requests, just change what is strictly necessary.

In practice we need to maintain the code on several branches and changing the way code looks make porting the code more difficult which means that the team porting and reviewing the code spend more time than necessary.

From now on we will close any PR that does not do the change advocated in the PR

@magicprinc
Copy link
Contributor Author

magicprinc commented Dec 1, 2023

@tsegismont I already knew the results, but I have made a benchmark with many explanations for you.
I have used your JMH defaults. I've removed Buffer from the benchmark to make the results loud and clear.

Benchmark                                 Mode  Cnt    Score   Error   Units
 baseline (almost no-op)                  thrpt  10  209,947 ± 0,449  ops/ms
 original vert.x copy-in-for-loop version thrpt  10    1,356 ± 0,005  ops/ms
 original version with micro-optimization thrpt  10    2,501 ± 0,352  ops/ms
 my JDK version 1                         thrpt  10   24,501 ± 0,357  ops/ms
 my JDK version 2                         thrpt  10   24,387 ± 0,067  ops/ms

@magicprinc
Copy link
Contributor Author

magicprinc commented Dec 1, 2023

@vietj Don't get me wrong: you ignore my PRs or take only few bits from them, and I don't have too much time to spend on this project. We are mostly a "Spring shop", but I hate WebFlux and try to remove it (we have no WebFlux experts and a lot of bug reports). With Vert.x and Virtual Threads, I am on the winning streak, but my colleagues are still very cautious.

So please 🙏 accept (with modifications)
this #4957 (I especially need this protected Object metric) and this vert-x3/vertx-web#2521

maybe these (mostly aesthetic) #4980 and public static <E extends Throwable> RuntimeException throwAsUnchecked(Throwable t) throws E {+throw Utils.throwAsUnchecked

And you won't see a PR from me 🥂 (only issues 😅)

@magicprinc
Copy link
Contributor Author

magicprinc commented Dec 1, 2023

@vietj If we speak about issues:
#4990 and #4971 would be great

I won't add it to issues, but more integration with https://github.com/smallrye/smallrye-fault-tolerance or vert.x alternative would be great.

BTW, We started to use Mutiny, but after some time returned to "bare" Vert.x with occasional Mutiny and some bits from it (mutiny-runtime and mutiny-zero). No offense to Mutiny intended: Mutiny is a breeze of fresh air compared to Spring Reactor/WebFlux.

PS: Vert.x 4.5.1 with Jackson Pool is a hot subject.

And thank you very much for such a great project!

@vietj
Copy link
Member

vietj commented Dec 3, 2023

@magicprinc then just provide PR that go straight to the point and don't touch code surrounded, we like contribution but we love when they are easy to review and easy to backport.

@vietj
Copy link
Member

vietj commented Dec 3, 2023

also please avoid remerging the main branch in your PR, rather rebase your PR otherwise again it's complicated to port.

@magicprinc
Copy link
Contributor Author

@vietj Please forgive me (and somehow merge) these times. I am really out of new ideas and time, so no more PRs 🙏

@tsegismont
Copy link
Contributor

Squashed and merged in bcddbac

@tsegismont tsegismont closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants