Re: [PATCH v2 7/8] gpu: nova-core: add NVKV typed decoding

[Alexandre Courbot]

On Mon Sep 14, 2026 at 4:16 PM JST, Eliot Courtney wrote:
> On Mon Sep 14, 2026 at 4:04 PM JST, John Hubbard wrote:
>> On 9/13/26 11:55 PM, Eliot Courtney wrote:
>>> On Mon Sep 14, 2026 at 12:46 PM JST, Alexandre Courbot wrote:
>>>> On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:
>> ...
>>>>> +            fn visit(
>>>>> +                &mut self,
>>>>> +                key: $crate::gsp::nvkv::KeyId,
>>>>> +                index: $crate::gsp::nvkv::Index,
>>>>> +                value: $crate::gsp::nvkv::DecoderValue<'_>,
>>>>> +            ) -> ::kernel::error::Result<bool> {
>>>>> +                Ok(false
>>>>> +                    $( || $crate::gsp::nvkv::Schema::visit(&mut self.$field, key, index, value)? )*)
>>>>
>>>> Mmm looks like this is going to be `O(n)` with `n` being the number of
>>>> fields?
>>>>
>>>> This is ok for a first implementation but eventually I hope we can
>>>> switch to a more efficient dispatch.
>>> 
>>> I thought quite a bit about this while writing this code, since we need
>>> the escape hatch to imperative decode (custom Schema impl basically). To
>>> be able to get it down to a match on the key, we need to know ahead of
>>> time which keys a Schema will consume. That duplicates the info from the
>>> visit() implementation.
>>> 
>>> I thought up a few methods but it's unclear to me which one is best, so
>>> I just left it for now. Please LMK if you think this is urgent, I can
>>> try in a follow up to improve this. Here are my ideas (when I say O(1)
>>> lookup I mean modulo how the compiler decides to do it with the set of
>>> key IDs it gets):
>>> 
>>> 1. current code - just visit()
>>> pros: key source of truth not duplicates
>>> cons: O(field) visit as you say
>>> 
>>
>> Something about the visit pattern has always concerned me, and I think
>> it has to do with the fact that people have been told to use it in
>> all kinds of situations. But it's really only ideal for a few situations,
>> such as updated a bunch of objects on a display, for example.
>>
>> It's not a good fit here IMHO.
>>
>> Is it hard to do one of the other choices, up front? If it's easy,
>> it would be nice to skip this visit entirely. :)
>>
>> thanks,
>
> It's not too hard to do one of #2-4 now, it's just a bit of extra
> complication which is why I left it out. The performance gain is kinda
> minimal since this doesn't happen in a hot path.
>
> That said, all the options I mention here still use `visit`. #2-4 are
> all about adding extra info to help avoid having to call `visit` as much
> + let the compiler have more info so it can generate a jump table etc.
> That said, if you have an alternate approach, happy to hear it.

I think it's ok to go with the current approach for now and grant us
some more time to think about an optimization. As long as the shape of
the `nvkv_decode` macro doesn't change, the switch should be transparent
anyway (and even if it isn't, it's not a big deal).

Sure, an O(n) complexity doesn't look great, but we won't be dealing
with an exponentially growing number of fields to handle, and the host
CPU can be assumed to be blazingly fast to the point where optimizing
this is more an exercice in good software engineering than a practical
necessity.

Still, I want a very visible `TODO` so we don't forget about it. :)