GGraziadei commented on PR #8892:
URL: https://github.com/apache/storm/pull/8892#issuecomment-4945734419
Hi @rzo1,
Thanks for the detailed review and the feedback!
First, just for full transparency, I wanted to highlight that the overall
design reflects what we discussed in the dedicated thread, where you provided a
key contribution to shaping this approach.
I have just pushed a commit that addresses all your points. Interestingly,
to address most of the documentation, logging, and nitpick fixes, I ran an
experiment and let Claude handle them directly. I'm sure you're already
familiar with these tools, but the agent did a remarkably good job. It feels
like projects like Apache Magpie might really be just around the corner?
On my end, I focused on the micro-benchmarking to see if the bitmask for
`isControlStreamId` was truly justified on the hot path.
Here are the results of the JMH microbenchmark (2 forks, 10 measured
iterations per case, JDK 25, jvm warmup discarded) comparing the bitmask
against the simplified `startsWith("__") + Set.contains` approach across
representative stream ids:
- Data streams: Both implementations perform identically (~1 ns cached / ~4
ns fresh) because both quickly reject the string on the very first character
without hashing.
- Control streams (e.g., __tick): Performance is identical (~5 ns cached /
~11 ns fresh) since both eventually route to the same Set.contains lookup.
- Edge case: The bitmask only wins in the rare scenario of `__`-prefixed
non-control IDs that miss the mask (e.g., __foo is ~1.5 ns faster cached / ~4.9
ns faster fresh by skipping the hash). However, it is actually slower if those
filters don't catch the string and it has to hash anyway (e.g., __fabcd: 6.6 vs
5.7 ns cached, 12.9 vs 12.2 ns fresh).
Even if we consider a worst-case scenario where a sequence of these
edge-case IDs adds around 5 ns per check, across a standard path size of 10 to
100 hops (considering the order of magnitude), the cumulative latency increase
would be roughly ~500 ns. Given that per-tuple processing overhead is measured
in microseconds, this is completely negligible. Since real topologies are
dominated by user data streams and genuine control tuples, the ~30 lines of
fragile static-init machinery are optimizing a case that doesn't realistically
occur.
I completely agree that simplifying the code and using a standard HashSet is
the right call. The clean-up is included in the latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
