Avoid using Scala's Range to reduce allocations

[mdedetrich]

This PR is a performance improvement that Pekko (from fork of Akka) did to their internal parboiled2 which is now being upstreamed (for context see apache/pekko-http@58d8f48).

Note that this version has been changed to be more appropriate for upstreaming, i.e. rather than manually inlining the condition (i.e. apache/pekko-http@58d8f48#diff-403c5e6ae1c63f17319929eff0a1f26e3de854a49308f8a7bd2ec707de395ed0L173) instead I have opted to update the constants in CharPredicate directly (furthermore I have done the change to all CharPredicate's). That means the PR performance for this PR also implies that CharPredicate.from ends up getting inlined (needs to be verified?)

@jrudolph

[sirthias]

Hmm... I don't know whether this change really has any positive effect.
How does it reduce allocations?
And even if the new function based version gets fully inlined I don't currently see how it would perform better in any significant way, but it may well be that I'm missing sth.

[mdedetrich]

And even if the new function based version gets fully inlined I don't currently see how it would perform better in any significant way, but it may well be that I'm missing sth.

Assuming it does get inlined I would also expect the performance improvement to be fairly minor, but I guess its an improvement nonetheless? There is also a presumption that since this was done in the context of akka-http because it had some impact (either that or it was just really trivial low hanging fruit).

Should I write a JMH benchmark to validate both whether it gets fully inlined and also how much performance we are talking about?

[sirthias]

Should I write a JMH benchmark to validate both whether it gets fully inlined and also how much performance we are talking about?

Hmm... I'd certainly be interested in the results of this.
If there is no benefit I'd be inclined to simply not include this change as I kind of like the application of the range based CharPredicate here.
If it's not hard for you to quickly whip up a benchmark you'd definitely have my applause! :-)

[mdedetrich]

I will look into this tomorrow as I am also curious about how much impact this will have (and also if it will fully inline or not)

[jrudolph]

I also don't quite understand where this comes from? It wasn't an optimization we did in akka-http...

The thing I manually optimized was CharUtils.toLowerCase because that was in a very hot path in HttpHeaderParser and also that optimization was before removing CharPredicate extends (Char => Boolean) (which was the bigger improvement because it introduces boxing in some call paths) which may or may not have made that previous optimization obsolete.

[mdedetrich]

I also don't quite understand where this comes from? It wasn't an optimization we did in akka-http...

My mistake here if I wasn't clear, I meant that it was done in context of akka-http

The thing I manually optimized was CharUtils.toLowerCase because that was in a very hot path in HttpHeaderParser and also that optimization was before removing CharPredicate extends (Char => Boolean) (which was the bigger improvement because it introduces boxing in some call paths) which may or may not have made that previous optimization obsolete.

Thanks for providing the context, it sounds that the performance improvement in this PR might indeed by useless. I am a bit sick so I didn't have time to create a benchmark but later on I will whip up a quick one just to confirm (but just like you I am quite certain its going to have almost no impact).

[mdedetrich]

Now I actually understand what @jrudolph was talking about from jrudolph/incubator-pekko-http@67f46c3.

I am going to close this PR, if for whatever reason any of us change our mind we can always re-open it.