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

[Eliot Courtney]

On Mon Sep 14, 2026 at 9:06 PM JST, Alexandre Courbot wrote:
> 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. :)

Yeah, I think with the current shape, an optimization would be additive
(e.g. the discussed `accepts` idea) and not affect callers at all, just
implementers of Schemas.

I'll add the TODO. Note that all the optimization ideas I've suggested
so far have negative points w.r.t. good software engineering practice
because they introduce the footgun that your Schema must correctly
declare the keys that it consumes and underdeclaring will cause wrong
behaviour. Since (I hope) we will be mostly gluing together existing
Schemas using the macros, it should be fairly hard to hit this footgun
though. A non-visit based solution would need to handle the regkey case
(nvkv is stateful, so it's hard to handle it in a purely declarative
way). That's why what I suggest here is essentially an imperative core
with declarative handling bolted on using macros.

thanks all~