Add new lengthIs method (more usable than lengthCompare)

[NthPortal]

Fixes scala/collection-strawman#338

[Ichoran]

Since the point of lengthCompare is often performance, can you benchmark whether there is a performance hit? Otherwise this looks good to me, except that having both lengthCompare(3) < 2 and lengthCompare < 2 both compile is a potential gotcha. Maybe some bikeshedding would help in that regard. (checkLength?)

[julienrf]

YMMV but I’m not a big fan of introducing intermediate classes for the sole purpose of providing a DSL that reads nicely (a la ScalaTest). I’d rather go with xs.lengthLessThan(2) or xs length_< 2.

[Ichoran]

@julienrf - Me either, normally. But are you more of a fan of underscores in method names and hybrid alphanumeric-symbolic method names? Especially after all the effort of providing non-symbolic equivalents for the purely symbolic and trivial to understand methods in collections?

That would suggest the choices are either an intermediate class (hopefully zero-overhead) or lengthLessThans, the latter of which rather clutter up the API. Perhaps it is worth it nonetheless.

[NthPortal]

@Ichoran I agree that it could use some bikeshedding. Perhaps lengthIs?

[NthPortal]

lengthComparePretty uses the new value class methods, and lengthCompareUgly uses the current/old method. size is the size of the List, and len is the length compared against.

The results are:

[info] Benchmark                                      (len)  (size)  Mode  Cnt     Score    Error  Units
[info] LengthCompareOpsBenchmark.lengthComparePretty      1       0  avgt   20     3.678 ±  0.117  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty      1       1  avgt   20     4.678 ±  0.096  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty      1      10  avgt   20     4.799 ±  0.120  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty      1     100  avgt   20     4.701 ±  0.101  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty      1    1000  avgt   20     5.666 ±  0.398  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty    100       0  avgt   20     4.047 ±  0.317  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty    100       1  avgt   20     4.956 ±  0.221  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty    100      10  avgt   20    12.393 ±  0.233  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty    100     100  avgt   20   180.834 ±  1.978  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty    100    1000  avgt   20   181.846 ±  3.117  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty  10000       0  avgt   20     3.678 ±  0.059  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty  10000       1  avgt   20     4.673 ±  0.073  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty  10000      10  avgt   20    12.615 ±  0.370  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty  10000     100  avgt   20   181.690 ±  3.097  ns/op
[info] LengthCompareOpsBenchmark.lengthComparePretty  10000    1000  avgt   20  2358.139 ± 34.935  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly        1       0  avgt   20     3.418 ±  0.074  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly        1       1  avgt   20     4.379 ±  0.153  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly        1      10  avgt   20     4.327 ±  0.067  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly        1     100  avgt   20     4.423 ±  0.139  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly        1    1000  avgt   20     4.457 ±  0.179  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly      100       0  avgt   20     3.444 ±  0.120  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly      100       1  avgt   20     4.437 ±  0.114  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly      100      10  avgt   20    12.575 ±  0.791  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly      100     100  avgt   20   180.054 ±  4.202  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly      100    1000  avgt   20   182.318 ±  2.364  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly    10000       0  avgt   20     3.372 ±  0.040  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly    10000       1  avgt   20     4.429 ±  0.150  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly    10000      10  avgt   20    12.153 ±  0.162  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly    10000     100  avgt   20   183.222 ±  2.175  ns/op
[info] LengthCompareOpsBenchmark.lengthCompareUgly    10000    1000  avgt   20  2414.478 ± 75.108  ns/op

The current way of doing it is not even consistently cheaper, and for the smaller values, the new way was only 1ns slower in one of the benchmarks. So, if there is a performance hit (which is not clear to me), it's an extremely small one, and probably not noticeable.

TL;DR the cost seems to be negligible to non-existent.

[Ichoran]

👍 👍 from me, but different people may prefer different colors of shed.

I think it is a very usable and intuitive API this way--you get only-as-much-as-needed evaluation, yet you can use the slightly-higher-level concepts you're already familiar with, and with only one thing to learn.

With lengthCompare there's always the momentary confusion about what negative means (length is shorter than test value or test value is less than length?), and with length_< and friends you have a visual code-parsing glitch every time you see it because of the rarity of foo_!?!-style methods, not to mention the overloading of _.

[NthPortal]

I also tweaked lengthCompare to check knownSize while I was in the area, as it seemed like a low-cost, high-value addition.

[NthPortal]

Is it worth @inlineing the methods as well?

[lrytz]

I like this a lot, and definitely prefer it over adding multiple methods with underscores in their names.

[szeiger]

I seem to be approaching this from a different angle than @Ichoran. This looks like a daunting amount of complexity for very little gain. The resulting syntax is very nice but getting to that syntax is anything but nice. The necessary machinery shows up in scaladoc and can leak out in error messages.

IMHO the goal of any syntactical sugar for lengthCompare should be to make it more discoverable. But then again, I am approaching this from a different angle because I don't find lengthCompare hard to use at all. It appears quite natural to me that xs.lengthCompare(i) op 0 is the same as xs.length op i for every comparison operator op.

[Ichoran]

I don't think that's "quite natural" to everyone, and even if it is natural, it's one extra mental transformation instead of zero. I guess it could be made more natural to everyone if the docs say exactly what you said (that is, xs.lengthCompare(i) op 0 is exactly the same as xs.length op i). Not sure that's enough.

[NthPortal]

squashed

[NthPortal]

Are we moving forward with this?

[NthPortal]

Is it worth @inlineing the methods as well?

Elaborating on this question, there are two possible sets of methods to inline: {lengthIs} and {<, <=, ==, !=, >=, >}. Which, if any, should be inlined?

[julienrf]

It seems that your benchmarks show no overhead for lengthIs vs lengthCompare, do they? But we can @inline everything (the lengthIs method and the comparison operators) if that doesn’t hurt…

[NthPortal]

It seems that your benchmarks show no overhead for lengthIs vs lengthCompare, do they?

For trivially small collections (only a couple of elements), it seems like there's maybe a cost of around a third of a nanosecond (ish), which on my processor, is about 1 cycle. For non-trivially small collections (where using lengthCompare instead of length actually matters), the noise exceeds any observable difference.

[NthPortal]

squashed with @inlines

[lrytz]

Thanks, @NthPortal!

[NthPortal]

yw! 😊