RFR: 8304138: [JVMCI] Test FailedSpeculation existence before appending. [v3]

[Yudi Zheng]

On Fri, 17 Mar 2023 16:08:38 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Yes, it would do what you are saying but I don't like infinite loops like that with conditional exists.
>> How big length of `failed_speculations_address` list you can have?
>> Consider implementing it as recursive method if depth is not big:
>> 
>> bool FailedSpeculation::add_failed_speculation(nmethod* nm, FailedSpeculation** failed_speculations_address, address speculation, int speculation_len) {
>>   assert(failed_speculations_address != nullptr, "must be");
>>   guarantee_failed_speculations_alive(nm, failed_speculations_address);
>> 
>>   size_t fs_size = sizeof(FailedSpeculation) + speculation_len;
>>   FailedSpeculation* fs = new (fs_size) FailedSpeculation(speculation, speculation_len);
>>   if (fs == nullptr) {
>>     // no memory -> ignore failed speculation
>>     return false;
>>   }
>>   guarantee(is_aligned(fs, sizeof(FailedSpeculation*)), "FailedSpeculation objects must be pointer aligned");
>>   
>>   if (!add_failed_speculation_recursive(failed_speculations_address, fs)) {
>>     delete fs;
>>     return false;
>>   }
>>   return true;
>> }
>> 
>> bool add_failed_speculation_recursive(FailedSpeculation** cursor, FailedSpeculation* fs) {
>>   if (*cursor == nullptr) {
>>     FailedSpeculation* old_fs = Atomic::cmpxchg(cursor, (FailedSpeculation*) nullptr, fs);
>>     if (old_fs == nullptr) {
>>       // Successfully appended fs to end of the list
>>       return true;
>>     }
>>     guarantee(*cursor != nullptr, "cursor must point to non-null FailedSpeculation");
>>   }
>>   // check if the current entry matches this thread's failed speculation
>>   int speculation_len = fs->data_len();
>>   if ((*cursor)->data_len() == speculation_len && memcmp(fs->data(), (*cursor)->data(), speculation_len) == 0) {
>>     return false;
>>   }
>>   return add_failed_speculation_recursive((*cursor)->next_adr(), fs);
>> }
>
>> @vnkozlov your suggestion eagerly allocates a new `FailedSpeculation`. I'm also generally allergic to infinite loops but I don't want to ever have to worry about a stack overflow in this code as it will crash the VM. I think we should leave Yudi's code in its current form.
> 
> Okay. You are the "boss" for this code ;^)

@vnkozlov @dougxc thanks for the review! I will keep it as is then.

-------------

PR: https://git.openjdk.org/jdk/pull/13022