Skip to content

Conversation

@arnetheduck
Copy link
Member

Using func avoids a template expansion at every usage site and just looks nicer .. unlike earlier nim versions, good code is generated for lent.

An additional benefit is that mutability semantics are preserved and the resulting value is immutable even when being accessed indirectly (unlike addr+[] which results in a var).

No matter what, when iterating over validators we continue to be affected by:

Using `func` avoids a template expansion at every usage site and just
looks nicer .. unlike earlier nim versions, good code  is generated for
`lent`.

An additional benefit is that mutability semantics are preserved and the
resulting value is immutable even when being accessed indirectly (unlike
`addr`+`[]` which results in a `var`).

No matter what, when iterating over validators we continue to be
affected by:

* nim-lang/Nim#25470
* nim-lang/Nim#25469
@tersec
Copy link
Contributor

tersec commented Jan 29, 2026

One nice aspect of the slight ugliness of getStateField is that it is somewhat involved:

template withState*(x: ForkedHashedBeaconState, body: untyped): untyped =
case x.kind
of ConsensusFork.Gloas:
const consensusFork {.inject, used.} = ConsensusFork.Gloas
template forkyState: untyped {.inject, used.} = x.gloasData
body
of ConsensusFork.Fulu:
const consensusFork {.inject, used.} = ConsensusFork.Fulu
template forkyState: untyped {.inject, used.} = x.fuluData
body
of ConsensusFork.Electra:
const consensusFork {.inject, used.} = ConsensusFork.Electra
template forkyState: untyped {.inject, used.} = x.electraData
body
of ConsensusFork.Deneb:
const consensusFork {.inject, used.} = ConsensusFork.Deneb
template forkyState: untyped {.inject, used.} = x.denebData
body
of ConsensusFork.Capella:
const consensusFork {.inject, used.} = ConsensusFork.Capella
template forkyState: untyped {.inject, used.} = x.capellaData
body
of ConsensusFork.Bellatrix:
const consensusFork {.inject, used.} = ConsensusFork.Bellatrix
template forkyState: untyped {.inject, used.} = x.bellatrixData
body
of ConsensusFork.Altair:
const consensusFork {.inject, used.} = ConsensusFork.Altair
template forkyState: untyped {.inject, used.} = x.altairData
body
of ConsensusFork.Phase0:
const consensusFork {.inject, used.} = ConsensusFork.Phase0
template forkyState: untyped {.inject, used.} = x.phase0Data
body

i.e. being a kind of explicit call helps discourage a bunch of getStateFields near each other on the same state, while it would be better to have one withState covering them all. This PR hides the ugliness but it's a factory for spawning lots of unnecessary separate case statements/expressions on the fork.

@arnetheduck
Copy link
Member Author

factory for spawning lots

yeah I thought about this but I think it's negligible - specially given the fact that all fields are at the same offset meaning that effectively, the case should go away, or almost at least. If there was any significant computation involved, I'd certainly agree.

@arnetheduck
Copy link
Member Author

also, we don't use Forked* in tight loops anyway, typically .. ie everywhere where performance matters, we go Forked

@github-actions
Copy link

Unit Test Results

       12 files  ±0    2 440 suites  ±0   51m 2s ⏱️ +23s
12 669 tests ±0    9 192 ✔️ ±0    3 477 💤 ±0  0 ±0 
63 860 runs  ±0  51 430 ✔️ ±0  12 430 💤 ±0  0 ±0 

Results for commit edece56. ± Comparison against base commit f896feb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants