RFR: 8236641: Improve Set.of(...).iterator() warmup characteristics

[John Rose]

> On Jan 15, 2020, at 5:46 PM, Stuart Marks <stuart.marks at oracle.com> wrote:
> 
> 
> 
>> On 1/13/20 8:03 AM, Claes Redestad wrote:
>> we're doing plenty of iterations over Set.of() instances during
>> bootstrap, which makes these operations show up prominently in
>> startup profiles. This patch refactors a few hot methods to get a
>> measureable startup improvement without regressing on targeted
>> microbenchmarks.
>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8236641
>> Webrev: http://cr.openjdk.java.net/~redestad/8236641/open.00/
> 
> OK, interesting and promising. A potential pitfall and a nitpick, though perhaps an important one.
> 
>  65     static {
>  66         long color = 0x243F_6A88_85A3_08D3L; // pi slice
>  67         long seed = System.nanoTime();
>  68         int salt = (int)((color * seed) >> 16);  // avoid LSB and MSB
>  69         REVERSE = salt >= 0;
>  70         // Force SALT to be positive so we can use modulo rather
>  71         // than Math.floorMod
>  72         SALT = Math.abs(salt);
>  73     }
> 
> After this code runs I'm not convinced that SALT will be positive... consider Integer.MIN_VALUE! Maybe just mask off the top bit instead? Thirty-one bits of salt should be sufficient for our purposes here.

Good catch.  Should be:

SALT = salt >>> 1;

Pleasingly it uses exactly the 32 rando-bits. 

> 
> In SetNIterator::next there is this code:
> 
> 
> 789                     do {
> 790                         if (REVERSE) {
> 791                             if (++idx >= len) {
> 792                                 idx = 0;
> 793                             }
> 794                         } else {
> 795                             if (--idx < 0) {
> 796                                 idx = len - 1;
> 797                             }
> 798                         }
> 799                     }
> 800                     while ((element = elements[idx]) == null);
> 
> Ah, the rare do-while loop! I think this makes sense, but it reads oddly, because it looks like the trailing "while" starts a new statement when in fact it's part of the do-while statement. The trailing semicolon then makes this look like a mistake. I'd suggest merging 799 and 800 so the last line of the loop looks like this:
> 
> 799                     } while ((element = elements[idx]) == null);
> 
> It's a small thing, but the "} while" on a line should be a tip-off that this is the rare do-while loop and not something else.

+1

> 
> Thanks,
> 
> s'marks