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

[Vladimir Kozlov]

On Fri, 17 Mar 2023 00:05:59 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Yudi Zheng has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   avoid duplicated entry.
>
> 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 ;^)

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

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