RFR (S): CR 8009120: Fuzz instruction scheduling in Hotspot compilers

[Vladimir Kozlov]

On 3/1/13 1:32 AM, Aleksey Shipilev wrote:
> Thanks!
>
> Here's the updated webrev:
>   http://cr.openjdk.java.net/~shade/8009120/webrev.02/
>
> Testing:
>   - release/fastdebug builds on my laptop: concurrency tests are failing
> with -XX:+StressLCM, as expected
>   - hotspot-comp JPRT is running now; I'll report on success/failure
>
> Comments below:
>
> On 03/01/2013 12:46 AM, Vladimir Kozlov wrote:
>> First, about your fuzzing code. fuzz_accept() will always return 'true'
>> for count == 1, after that probability of 'true' will decrease with each
>> count's increment. So you will always select first candidate which is
>> not random. Why you don't want a uniform distribution? Also why you want
>> only one candidate to be selected (if I understand code correctly)?
>
> There is a confusion about the contract for this method. phase.hpp
> before, and compile.cpp now has the example of how this method is used.
> Both LCM and GCM are not storing the list of candidates, which we then
> can magically use to choose from uniformly.
>
> Instead, they update the best candidate as they munch through the
> candidate list, and we even don't know in advance how much candidates
> there'll be. In this scheme, you need to adjust the randomicity, or else
> you will end up biasing the selection towards the latter candidates.
>
> Quick back-envelope calculation shows that for the list of n candidates
> the equal probability for the candidate to persist as "best" can be
> achieved by replacing it with "next" k-th candidate with the probability
> of 1/k. It can be easily shown that by the end of the run, the
> probability for all candidates are converged to 1/n, thus giving the
> uniform distribution among all the candidates.

I see your point now. I would suggest to put this explanation (last 
paragraph) into randomized_select() comment.

>
>> Could you change "fuzzing", "fuzz" to "stressing", "stress" in flags
>> names and comments. It will be in the line with other stress flags/code
>> we have already.
>
> Although I think "fuzz" declares the intent more clearly, I changed
> everything to "stress".
>
>> I also think fuzz_accept() could be different name, may be
>> fuzz_selected(cand) or something?
>
> "randomized_select" it is now.

Thanks. Add assert(count > 0, "only positive") into randomized_select().

>
>> define flags as 'develop' (not 'notproduct') so they will be accessible
>> (but not modifiable) in product code.
>
> Yes, cool.
>
>> bool fuzz_candidate = FuzzGCM && fuzz_selected(++fuzz_candidate_cnt);
>>
>>        if (LCA_freq < least_freq     || // Better Frequency
>> +         fuzz_candidate ||
>>            ( !in_latency          &&    // No block containing latency
>
> Nope, that defeats the purpose. I want the latency-based policy to be
> disabled at all when stressing is enabled. Otherwise both LCM and GCM
> will converge to lowest latency, and stressing will have the nil effect.
> Hence I left it as is, with develop flag cleanup.

Thank you for clarification. Could you remove spaces in the condition in 
lcm.cpp? We are cleaning up code style:

508     if (choice < n_choice ||
509         (choice == n_choice &&
510          ((StressLCM && Compile::randomized_select(cand_cnt)) ||
511           (!StressLCM &&
512            (latency < n_latency ||
513             (latency == n_latency &&
514              (score < n_score))))))) {

Ask Vladimir Ivanov to be your sponsor.

Thanks,
Vladimir

>
>> Phase class is bad place for fuzz_accept() method. We usually put such
>> methods into Compile class if you want to use this method in other
>> places in C2.
>
> I was wondering where to put it. It is in the Compile now.
>
>> Why you need C2_PHASE_ in C2_PHASE_FUZZ_DOMAIN name? Do we have other
>> FUZZ_DOMAIN? You don't need to worry about that if you move method body
>> and FUZZ_DOMAIN definition into .cpp file.
>
> Fixed.
>
>> Also why not choose 2^n value so that '%' could be simple masking.
>
> Improved.
>
> -Aleksey.
>