Headers +toString(), fixes #1385

magicprinc · 2025-08-10T21:30:29Z

Headers.java

I actually came to add toString(), required for debugging and logging;

Then I saw and:

Fixed nullability issues.
Replaced inefficient keySet() + Map.get() pattern with entrySet().
Fixed broken forEach loop; added a failing test to confirm the issue before the fix.
Replaced val.chars() with a more efficient for-loop and charAt().
Removed "redundant" private final boolean readOnly field; replaced with another check.
Finally, I have added missing tests.

I really don't have time for several one-line PRs 😭🙏
This PR is obvious and not bigger than your recent PRs.
I open to your questions, critic, and recommendations for improvement 🤝

PS: Some java quirks (why I have changed those lines)

Collections.unmodifiableMap isn't completely unmodifiable
java.util.Collections.UnmodifiableMap#forEach

m.forEach(action); // 😭🤷‍♀️

java.util.Objects#hash

public static int hash(Object... values) {
  return Arrays.hashCode(values);
}

it is overkill for ONE value

For one value, it is recommended to use
java.util.Objects#hashCode

public static int hashCode(Object o) {
  return o != null ? o.hashCode() : 0;
}

The Message of the thrown exception is probably unexpected
~ "Header value has invalid character: 42911"
c has type int ⇒ "+ c " creates decimal digit representation
but "+ (char) c" can be problematic too: invisible characters, special characters e.g.
0x200B// Zero Width space
0x200C// Zero Width non-joiner
0x200D// Zero Width joiner

val.chars().forEach(c -> {
  if (c > 127 || c == 10 || c == 13) {
    throw new IllegalArgumentException(VALUE_INVALID_CHARACTERS + c);

…t; fix broken forEach; optimize; missing tests

magicprinc · 2025-08-10T22:11:25Z

PS: I think

private final Map<String, Integer> lengthMap;
private int dataLength;

can be optimized (removed).

If you accept this PR and give me the go-ahead: I can do this optimizations with a benchmark.

Anyway, not cached serializeToArray is used in io.nats.client.impl.NatsMessage#copyNotEmptyHeaders 😭😱

src/main/java/io/nats/client/impl/Headers.java

scottf · 2025-08-11T21:42:30Z

FYI, the code was merged with a PR from earlier today, so update your branch.

scottf · 2025-08-11T21:44:56Z

Collections.unmodifiableMap isn't completely unmodifiable
Ok, good fix.

return Arrays.hashCode(values);
Ok.

scottf · 2025-08-11T21:48:37Z

private final Map<String, Integer> lengthMap;
private int dataLength;

I'll look at any change, but this isn't broken. All that work was done to manually track the size as data was added and removed because the information is accessed repeatedly, and it prevented calculating it repeatedly. I suppose a benchmark could provide sufficient justification to accept a change.

scottf · 2025-08-11T21:50:53Z

The size of this pr is fine, since it's one class and all related. And yes I know my PR's are large a lot, but that's the nature of working something that affects many things, like nullability. It's a fine line and I'm sure I cross it sometimes.

scottf · 2025-08-11T22:00:01Z

The "not cached" serializeToArray is used to reduce copies of bytes.

1) getBytes 3-5x faster than getBytes(US_ASCII) 2) don't create and copy extra byte arrays

magicprinc · 2025-08-12T00:47:32Z

I have applied all your recommendations

Plus: new serializeToArray() is 3.7 times faster than previous one

src/main/java/io/nats/client/impl/Headers.java

scottf · 2025-08-12T11:05:12Z

src/main/java/io/nats/client/impl/Headers.java

@@ -522,7 +523,8 @@ private void checkKey(String key) {
 		for (int idx = 0; idx < len; idx++) {
 			char c = key.charAt(idx);
 			if (c < 33 || c > 126 || c == ':') {
-				throw new IllegalArgumentException(KEY_INVALID_CHARACTER + "'" + c + "'");
+				throw new IllegalArgumentException(KEY_INVALID_CHARACTER +"0x"+ Integer.toHexString(c)
+						+" at "+ idx +" in "+ key);


Just this please.

private static final String KEY_INVALID_CHARACTER = "Header key has invalid character: 0x"; throw new IllegalArgumentException(KEY_INVALID_CHARACTER + Integer.toHexString(c));

scottf · 2025-08-12T11:06:57Z

src/main/java/io/nats/client/impl/Headers.java

 			if (c > 127 || c == 10 || c == 13) {
-				throw new IllegalArgumentException(VALUE_INVALID_CHARACTERS + c);
+				throw new IllegalArgumentException(VALUE_INVALID_CHARACTERS +"0x"+ Integer.toHexString(c)
+						+" at "+ i +" in "+ val);


private static final String VALUE_INVALID_CHARACTERS = "Header value has invalid character: 0x"; throw new IllegalArgumentException(VALUE_INVALID_CHARACTERS + Integer.toHexString(c)));

scottf · 2025-08-12T11:12:13Z

src/main/java/io/nats/client/impl/Headers.java

+				b[i] = ';';
+			}
+		}
+		return new String(b, 0, HVCRLF_BYTES, b.length - HVCRLF_BYTES - 3);


I'm fully on board with squeezing every nano second out, but we can't use a deprecated method. So just simplify this. We are talking maybe nano seconds on an operation that probably isn't used all that much. Headers have been around for years and no one has asked for this, users probably write their own anyway.

@Override public String toString() { byte[] b = getSerialized(); for (int i = 0, len = b.length; i < len; i++) { switch (b[i]) { case CR: b[i] = ';'; break; case LF: b[i] = ' '; break; } } return new String(b, US_ASCII); }

new String(b, US_ASCII)→new String(b, ISO_8859_1) ⇒ 3x times faster

Yes, in this case deprecated String ctor isn't faster than new String(b, ISO_8859_1) 👍

src/main/java/io/nats/client/impl/Headers.java

scottf · 2025-08-12T11:25:45Z

Plus: new serializeToArray() is 3.7 times faster than previous one
Explain this like I'm bad at math.

magicprinc · 2025-08-12T12:06:58Z

I see... Github didn't publish my comments
Now they are published.

magicprinc · 2025-08-12T12:08:33Z

and

magicprinc · 2025-08-12T12:20:38Z

Plus: new serializeToArray() is 3.7 times faster than previous one
Explain this like I'm bad at math.

Special getBytes() allows serializing 3.7 times faster:
instead of a) "slow" US_ASCII encoder b) into new array:
getBytes() simply copies low bytes of chars into your array.

Keys and values are US_ASCII: the input is checked for that, so both methods generate the same bytes as an output.

The result is absolutely the same, but the speed is much faster.

I have added a simple benchmark in tests: you can see yourself 🤝

HeadersTests#benchmark_serializeToArray

On my computer
Old version: Time: 24622.87ms, Op/sec: 4061264
New version: Time: 6660.18ms, Op/sec: 15014614
15014614/4061264 = 3.7 more op/sec ⇒ 3.7x times faster 🚀

For long keys/values new version should be even faster

"Just this please. private static final String KEY_INVALID_CHARACTER = "Header key has invalid character: 0x"; throw new IllegalArgumentException(KEY_INVALID_CHARACTER + Integer.toHexString(c));"

magicprinc · 2025-08-12T12:43:00Z

✅

HashSet capacity › size: reasons are explained
entrySet vs keySet+get: reasons are explained
3.7 speed improvement: reasons are explained
checkKey, checkValue - validation messages are simplified after review
toString() - simplified after review
serializeToArray uses deprecated getBytes to achieve 3.7x speed improvement, see p3 and explanation in comments above
JDK 24 has this method, Microsoft uses this method, and... 3.7x is 3.7x

JDK25-26 has it:
https://github.com/openjdk/jdk/blob/a382996bb496d50b4eb5a6be9f61e5c2f8aaae2d/src/java.base/share/classes/java/lang/String.java#L1793

I have fixed and explained everything I had seen.
I open to your instructions 🤝

PS: more about deprecations
The code uses here and there String.getBytes() which is (IMHO) worse than using deprecated methods 😱

Javadoc:

* Encodes this {@code String} into a sequence of bytes using the
* platform's default charset, storing the result into a new byte array.
* <p> The behavior of this method when this string cannot be encoded in
* the default charset is unspecified.  The {@link

it is almost "random", can fail and usually slower than US_ASCII/ISO_8859_1

magicprinc · 2025-08-12T13:00:18Z

I think, It has sense to make checkKey and checkValue package-private or (better) put them into Checker class:

In old java versions like 8: synthetic accessor methods may be generated for private access, causing minor overhead.
And IDEA shows warnings (from OOP-stype point of view)

scottf · 2025-08-12T15:31:55Z

serializeToArray uses deprecated getBytes to achieve 3.7x speed improvement, see p3 and explanation in comments above
JDK 24 has this method, Microsoft uses this method, and... 3.7x is 3.7x

JDK25-26 has it:
public String(byte ascii[], int hibyte, int offset, int count) {

@deprecated This method does not properly convert bytes into characters.

I'd actually be fine with a manual implementation that is similar to deprecated code but is tuned to recognize that we have actually already validated the key in checkKey, so we know that every char/byte c is 33 <= c <= 126

Value is also validated. So in both places where you use a deprecated method, you can hand code the fastest anyway to take advantage of that knowledge.

src/main/java/io/nats/client/impl/Headers.java

scottf · 2025-08-12T16:48:19Z

put checkKey and checkValue into Checker class:

Sounds good.

magicprinc · 2025-08-12T19:36:11Z

with a manual implementation that is similar to deprecated code

Please don't 🙏
It is IMPOSSIBLE to beat JDK team in such tasks.
I have added an extra benchmark with manual for-loop-charAt-bytes-copy (example results are in JavaDoc):
https://github.com/magicprinc/MagicBenchmarks/blob/main/jnats/src/main/java/jnats/StringGetBytesBenchmark.java

Manual implementation is 116 (one hundred sixteen) times slower than String.getBytes 😱😱😱

This method is deprecated, not because it is bad or will be removed any time soon (not in years). There are no alternatives to it. Microsoft and others use it. It is deprecated as a reminder that "it is for professionals, who understand exactly what they are doing and where to use it exactly".

Please discuss it with your colleagues. Excessive formalism can be harmful 🙏

The closest competitor getBytes(ISO_8859_1) is 10 times slower.
Previous getBytes(US_ASCII) is 30x times slower.

All numbers are in the benchmark ^^^

magicprinc · 2025-08-12T19:54:54Z

A copy of my thoughts about HashMap(capacity formula) to be sure you see it

I see 4 possible solutions:

a simpler formula: size * 4 / 3 like in JDK
(best) a utility function in a utility class ~

/** long explanation from native english speaker and link to JDK with example */
int capacityForSize(int size) {
  return size * 4 / 3;
}

then new HashMap<>(capacityForSize(srcMap.size()))

load factor = 1: benchmarks are required! It can make everything worse.
HashMap uses closed addressing (!)

When two or more keys hash to the same bucket, a collision occurs.
Java HashMap resolves collisions by storing entries in a linked list (or, starting from Java 8, a balanced binary tree) at each bucket.
This means multiple key-value pairs can be stored at the same bucket location.
When inserting or retrieving a key, HashMap uses the key's hash code to find the bucket and then uses the key's equals() method to distinguish between entries in the bucket.
If the number of entries in a bucket grows beyond a threshold, the linked list is replaced with a balanced binary tree to improve performance from O(n) to O(log n) in worst cases.
With fill factor 1 collisions are more likely, so memory consumption will probably increase → tests and benchmarks

use srcMap.size() without *4/3 as capacity (it will be two internal hashtable-array allocations), but again benchmarks are required! It can make everything worse for small src-Maps

PS: if you open the sources of JDK:
In HashMap(Map) ctor, you see the same formula

final void putMapEntries(Map<? extends K, ? extends V> m, boolean evict) {
  int s = m.size();
  if (s > 0) {
    if (table == null) { // pre-size
      double dt = Math.ceil(s / (double)loadFactor);//👈 the same 👍
      int t = ((dt < (double)MAXIMUM_CAPACITY) ? (int)dt : MAXIMUM_CAPACITY);

scottf · 2025-08-12T19:59:18Z

about HashMap
It would be so rare if there is more than 1 entry. Yes, people can put multiple values in, but this is an edge case. Just leave it as the default and we can move on.

magicprinc · 2025-08-12T20:03:17Z

about HashMap
It would be so rare if there is more than 1 entry. Yes, people can put multiple values in, but this is an edge case. Just leave it as the default and we can move on.

The command is not understood. Please specify 🙏

1) static class Checker doesn't contain hidden reference to Headers ⇒ -1 field 👍 2) not private checkKey and checkValue can be accessed directly from Checker, without synthetic accessors 3) static should be a little faster

magicprinc · 2025-08-12T20:08:27Z

put checkKey and checkValue into Checker class:

Sounds good.

Done! 👍

scottf · 2025-08-12T20:31:06Z

about hashMap. Considering that there is almost never more than 1 entry in the map, the time we spent discussing this optimization costs more then the gain we will see. If you want to optimize considering that there is almost always 1 entry, I will look at that, otherwise, just leave the code using the default constructor.
About the getBytes. I really do not want to use a deprecated method, but it is ridiculously faster and I can't really ignore that. Considering that the key/value have already been validated, the reason for deprecation should not cause us any issues. I guess I don't understand why they even deprecated it but never removed it and I'll just assume that newer jdk implementations are fully aware of the issues and don't actually have the problem. I wouldn't mind the ignore deprecation annotation and 1 sentence comment with the annotation as to why we are doing this.

magicprinc · 2025-08-12T20:51:25Z

just leave the code using the default constructor

For small src maps this is the best solution 👍

I wouldn't mind the ignore deprecation annotation and 1 sentence comment with the annotation as to why we are doing this

Thank you! 💖

Done! 👍

scottf

LGTM

Headers: toString! fix Nullability; use entrySet instead of keySet+ge…

7da6682

…t; fix broken forEach; optimize; missing tests

scottf requested changes Aug 11, 2025

View reviewed changes

Merge branch 'main' into feature/Headers

13d38d0

magicprinc added 4 commits August 12, 2025 02:47

revert readOnly back to field

b13a224

refactoring after review

cd4f4d2

much faster serializeToArray

2475934

1) getBytes 3-5x faster than getBytes(US_ASCII) 2) don't create and copy extra byte arrays

benchmark: new serializeToArray is 3.7 times faster

19122d0

disable benchmark

6916871

scottf requested changes Aug 12, 2025

View reviewed changes

magicprinc added 3 commits August 12, 2025 15:21

Merge branch 'nats-io:main' into feature/Headers

d110801

simplify validation message after review

5893a7a

"Just this please. private static final String KEY_INVALID_CHARACTER = "Header key has invalid character: 0x"; throw new IllegalArgumentException(KEY_INVALID_CHARACTER + Integer.toHexString(c));"

simplify toString after review

3788bb1

scottf requested changes Aug 12, 2025

View reviewed changes

src/main/java/io/nats/client/impl/Headers.java Outdated Show resolved Hide resolved

scottf added 2 commits August 12, 2025 13:06

Merge branch 'main' into feature/Headers

bfdb487

Merge branch 'main' into feature/Headers

e23a805

magicprinc and others added 2 commits August 13, 2025 00:11

fix after review

29e3088

Merge branch 'main' into feature/Headers

9f528a2

scottf approved these changes Aug 14, 2025

View reviewed changes

scottf merged commit 2b38c86 into nats-io:main Aug 14, 2025
1 check passed

Headers +toString(), fixes #1385

Headers +toString(), fixes #1385

Uh oh!

Conversation

magicprinc commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magicprinc commented Aug 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scottf commented Aug 11, 2025

Uh oh!

scottf commented Aug 11, 2025

Uh oh!

scottf commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottf commented Aug 11, 2025

Uh oh!

scottf commented Aug 11, 2025

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

Uh oh!

scottf Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

scottf Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

scottf Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

magicprinc Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scottf commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magicprinc commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

scottf commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

scottf commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottf commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

magicprinc commented Aug 12, 2025

Uh oh!

scottf commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magicprinc commented Aug 10, 2025 •

edited

Loading

scottf commented Aug 11, 2025 •

edited

Loading

magicprinc commented Aug 12, 2025 •

edited

Loading

magicprinc commented Aug 12, 2025 •

edited

Loading

scottf commented Aug 12, 2025 •

edited

Loading

magicprinc commented Aug 12, 2025 •

edited

Loading

scottf commented Aug 12, 2025 •

edited

Loading

magicprinc commented Aug 12, 2025 •

edited

Loading