[alshabib]

Currently, retrieveNodeList iterates over all map keys via rv.MapKeys() and invokes getKeyValue (which reflects over every struct field and parses YANG path struct tags) for every existing list element on each ytypes.SetNode / retrieveNode call. Inserting N elements into a YANG list therefore performs O(N^2) struct-tag parses and reflection lookups.

For single-keyed lists (where the map key is not a struct) and non-wildcard/non-partial-key queries:

1. Convert the gNMI path key string to the map key's reflect.Type via StringToType and perform a direct O(1) rv.MapIndex(kfv) lookup.
2. When the element is absent and modifyRoot is set, insert the new entry directly via insertAndGetKey without scanning existing keys.
3. For types where StringToType cannot directly construct the map key (e.g., union interface keys), pre-compute the key field struct index once outside the rv.MapKeys() loop instead of calling getKeyValue on every element.

[coveralls]

coverage: 88.444% (-0.3%) from 88.758% — optimize-single-key-list-lookup into master

[robshakir]

This code is a bit of a mess. I'll add comments throughout.

[robshakir]

I'm not sure why this is an else if -- if the list key type was a struct (as above), then we handle the fields above. I understand they are mutually exclusive, but it seems like we should make this clearer to understand (at least with some comments).

[robshakir]

I'd suggest here that we have a check that says:

if rv.Len() == 0 {
  // we've got a 0 length map the root value -- it seems
  // like we should just be able to return nothing since there are
  // no nodes that match the keys that we specified.
}

switch {
case util.IsTypeStruct(listKeyT):
  // handle struct type
default:
  // we know here that we have a length > 0 so we can just go ahead and handle this case.
}

This needs checking though, since there seems to be a case that we're handling transitory cases later down the function -- but still, if there's nothing in the map, we don't have any list elements to traverse.

[robshakir]

We should prefer to handle the err != nil case here, and understand what it is we want to do at that point.

Almost all paths inside this function seem to return just very late in the code -- not close to when we do the check.

[robshakir]

I think this function should be refactored out -- since it seems like we're saying that there's a shortcut that we can take when we're not handling wildcard keys.

Can you split the function that we have:

func retrieveNodeList(...) {
  // validity checks

  switch {
  // check whether we are handling wildcards
  case noWildcards:
     return retrieveNodeListConcrete(...)
  case wildCards:
     return retrieveNodeListWildcard(...)
  }
}

Since this doesn't add a bunch of lines into an already very long function.

[robshakir]

@alshabib Can you PTAL at #1112 and #1113 ? I had some strong feelings about code structure, and implemented them here -- PTAL as to whether this meets your requirement. If so, can you review that code?

[robshakir]

TLDR: I created #1112 as an alternative to #1111, and I added benchmarks in #1113. Across all benchmarks, #1113 is ~39% faster than #1111 and uses ~38% fewer allocations.

• Single-key lists (string, integer, enum): ytypes: optimize single-key list lookup in retrieveNodeList to O(1) #1111 and ytypes: add list lookup benchmarks and 5% allocation regression tests #1113 have the same speed and allocations. Both do a direct O(1) MapIndex lookup.
• Multi-key lists: ytypes: optimize single-key list lookup in retrieveNodeList to O(1) #1111 does not change multi-key lists, so each lookup still scans the full map (O(N)). ytypes: refactor retrieveNodeList and optimise concrete list lookups to O(1) #1112 builds the key struct directly and does an O(1) MapIndex lookup. At N = 1000, ytypes: add list lookup benchmarks and 5% allocation regression tests #1113 is 4.6x to 15.8x faster.
• decimal64 and defined-type keys: ytypes: optimize single-key list lookup in retrieveNodeList to O(1) #1111 ignores StringToType errors, so decimal64 (float64) and defined scalar keys fall back to an O(N) scan. ytypes: refactor retrieveNodeList and optimise concrete list lookups to O(1) #1112 adds these types to StringToType so lookups stay O(1) (12x to 38x faster at N = 1000). Invalid keys also return an error immediately and do not scan the map.

benchstat Results at N = 1000 (#1111 vs. #1113)

                                           │   pr1111    │               pr1113                │
                                           │   sec/op    │   sec/op     vs base                │
SetNodeSingleKeyList/n=1000-16               35.11m ±22%   34.35m ± 7%        ~ (p=0.937 n=6)
GetNodeSingleKeyList/n=1000-16               22.67µ ± 3%   23.27µ ±13%        ~ (p=0.180 n=6)
GetOrCreateNodeSingleKeyList/n=1000-16       23.91µ ± 4%   25.61µ ±57%        ~ (p=0.093 n=6)
DeleteNodeSingleKeyList/n=1000-16            4.506m ± 4%   4.418m ± 4%        ~ (p=0.310 n=6)
SetNodeMultiKeyList/n=1000-16               233.66m ±17%   51.03m ±13%  -78.16% (p=0.002 n=6)
GetNodeMultiKeyList/n=1000-16               390.61µ ±13%   30.54µ ±32%  -92.18% (p=0.002 n=6)
GetOrCreateNodeMultiKeyList/n=1000-16       392.05µ ±16%   31.59µ ±13%  -91.94% (p=0.002 n=6)
DeleteNodeMultiKeyList/n=1000-16           125.978m ± 5%   7.969m ±12%  -93.67% (p=0.002 n=6)
SetNodeDecimal64SingleKeyList/n=1000-16     259.11m ± 4%   20.55m ± 7%  -92.07% (p=0.002 n=6)
GetNodeDecimal64SingleKeyList/n=1000-16    279.912µ ±68%   9.519µ ± 2%  -96.60% (p=0.002 n=6)
DeleteNodeDecimal64SingleKeyList/n=1000-16 138.037m ± 4%   3.599m ± 2%  -97.39% (p=0.002 n=6)

@alshabib -- please can you review #1112 and #1113 and let me know what you think?

Thanks,
r.

[robshakir]

Closing as we merged the other implementation.