Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NRVO does not occur with init procedures #19094

Closed
Astavie opened this issue Nov 2, 2021 · 1 comment · Fixed by #19462
Closed

NRVO does not occur with init procedures #19094

Astavie opened this issue Nov 2, 2021 · 1 comment · Fixed by #19462

Comments

@Astavie
Copy link

Astavie commented Nov 2, 2021

I've read that using result guarantees NRVO to take place, so I was hoping that in the example initX and initXInPlace would be equivalent, resulting in no copying of type X resulting in innerAddress and outerAddress staying the same
However it seems that doesn't happen, because test "NRVO1" fails while "NRVO2" succeeds

I added the filler field just to check if the object needed to be of a sufficient size, but no luck there either

Example

import unittest

type
  X = object
    filler: array[2048, int]
    innerAddress: uint

proc initX(): X =
  result.innerAddress = cast[uint](result.addr)

proc initXInPlace(x: var X) =
  x.innerAddress = cast[uint](x.addr)

test "NRVO1":
  var x = initX()
  let innerAddress = x.innerAddress
  let outerAddress = cast[uint](x.addr)
  check(innerAddress == outerAddress) # [FAILED]

test "NRVO2":
  var x: X
  initXInPlace(x)
  let innerAddress = x.innerAddress
  let outerAddress = cast[uint](x.addr)
  check(innerAddress == outerAddress) # [OK]

Current Output

innerAddress was 140730050675008
outerAddress was 94589955355680
[FAILED] NRVO1
[OK] NRVO2

Expected Output

[OK] NRVO1
[OK] NRVO2

Possible Solution

Additional Information

Tested on the latest stable and development releases. Also tested on version 1.2.0 and 1.4.0 of the compiler, which also have the same issue.

$ nim -v
Nim Compiler Version 1.7.1 [Linux: amd64]
Compiled at 2021-11-01
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: 2f730afe9e7eb981258aea257deba0240f488dd1
active boot switches: -d:release
@arnetheduck
Copy link
Contributor

type
  SomeType = object
    a, b, c, d: seq[byte]


proc xxxxxx(): SomeType =
  var res = SomeType(a: @[])
  res

debugEcho xxxxxx()

Simplified example I just ran into - this generates the following code which is .. quite bad (nim 1.2.14):

N_LIB_PRIVATE N_NIMCALL(void, xxxxxx__eBuu7pI70biYLIU048cjuw)(tyObject_SomeType__t9b4CnNFTxaAmAeIEYM69azg* Result) {
        tyObject_SomeType__t9b4CnNFTxaAmAeIEYM69azg res;
        nimfr_("xxxxxx", "/home/arnetheduck/status/nimbus-eth2/beacon_chain/testit.nim");
        nimln_(7, "/home/arnetheduck/status/nimbus-eth2/beacon_chain/testit.nim");
        nimZeroMem((void*)(&res), sizeof(tyObject_SomeType__t9b4CnNFTxaAmAeIEYM69azg));
        chckNil((void*)(&res));
        nimZeroMem((void*)(&res), sizeof(tyObject_SomeType__t9b4CnNFTxaAmAeIEYM69azg));
        res.a = NIM_NIL;
        genericSeqAssign((&(*Result).a), res.a, (&NTI__6H5Oh5UUvVCLiakt9aTwtUQ_));
        genericSeqAssign((&(*Result).b), res.b, (&NTI__6H5Oh5UUvVCLiakt9aTwtUQ_));
        genericSeqAssign((&(*Result).c), res.c, (&NTI__6H5Oh5UUvVCLiakt9aTwtUQ_));
        genericSeqAssign((&(*Result).d), res.d, (&NTI__6H5Oh5UUvVCLiakt9aTwtUQ_));
        popFrame();
}

arnetheduck added a commit to status-im/nimbus-eth2 that referenced this issue Nov 15, 2021
These tools allow modifying an existing nimbus database for the purpose
of recovery or reorg, moving the head, tail and genesis to arbitrary
points.

* remove potentially expensive `putState` in `BeaconStateDB`
* introduce `latest_block_root` which computes the root of the latest
applied block from the `latest_block_header` field (instead of passing
it in separately)
* avoid some unnecessary BeaconState copies during init
* discover nim-lang/Nim#19094
* prefer `HashedBeaconState` in a few places to avoid recomputing state
root
* fetch latest block root from state when creating blocks
* harden `get_beacon_proposer_index` against invalid slots and document
* move random spec function tests to `test_spec.nim`
* avoid unnecessary state root computation before block proposal
arnetheduck added a commit to status-im/nimbus-eth2 that referenced this issue Nov 18, 2021
* ncli_db: add putState, putBlock

These tools allow modifying an existing nimbus database for the purpose
of recovery or reorg, moving the head, tail and genesis to arbitrary
points.

* remove potentially expensive `putState` in `BeaconStateDB`
* introduce `latest_block_root` which computes the root of the latest
applied block from the `latest_block_header` field (instead of passing
it in separately)
* avoid some unnecessary BeaconState copies during init
* discover nim-lang/Nim#19094
* prefer `HashedBeaconState` in a few places to avoid recomputing state
root
* fetch latest block root from state when creating blocks
* harden `get_beacon_proposer_index` against invalid slots and document
* move random spec function tests to `test_spec.nim`
* avoid unnecessary state root computation before block proposal
ringabout added a commit to ringabout/Nim that referenced this issue Jan 28, 2022
Araq pushed a commit that referenced this issue Jan 29, 2022
* [add testcase] NRVO does not occur with init procedures

close #19094

* Update tests/ccgbugs2/tcodegen.nim
PMunch pushed a commit to PMunch/Nim that referenced this issue Mar 28, 2022
* [add testcase] NRVO does not occur with init procedures

close nim-lang#19094

* Update tests/ccgbugs2/tcodegen.nim
narimiran pushed a commit that referenced this issue Apr 24, 2023
* [add testcase] NRVO does not occur with init procedures

close #19094

* Update tests/ccgbugs2/tcodegen.nim

(cherry picked from commit 33cd883)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants