Multiple SSTORE calls for the same storage location

[ngotchac]

Solidity version

I tested a couple of version from 0.4.0 onwards.

Source code

pragma solidity ^0.4.x;

contract TestSStore {
    struct TestStruct {
        uint128 a;
        uint128 b;
    }
    
    TestStruct s_data;
    
    function store (uint128 _a, uint128 _b) external {
      s_data = TestStruct({ a: _a, b: _b });
    }
}

Steps to reproduce

Just deploy the contract, call store method with non-0 values, and look at the executed OPCODES

Result

It executed two SSTORE OPCODES

Expected Behaviour

As the Struct is composed of 2 uint128, it fits in one 32-bytes area. However, when calling store, it executes two SSTORE opcodes, thus increasing the gas usage. This increases the gas usage by 5,000 (since it just re-writes on the some storage location), which could be improved.

[androlo]

User @adamskrodzki raised this issue on gitter as well, with an even more complex struct. It does 8 writes (1 full price + 7 discounted) for this:

    struct Bet {
        uint8 num1;
        uint8 num2;
        uint8 num3;
        uint8 num4;
        uint8 num5;
        uint8 num6;
        address gambler;
        uint32 betSum;
    }

No issues with packing though, they all end up in the same storage slot.

This i can confirm on latest stable (0.4.17+commit.bdeb9e52.Emscripten.clang) + nightly (0.4.18-nightly.2017.10.2+commit.c6161030.Emscripten.clang) of the emscripten version of solc, with optimizer enabled.

contract GameFundTest{

    struct Bet {
        uint8 num1;
        uint8 num2;
        uint8 num3;
        uint8 num4;
        uint8 num5;
        uint8 num6;
        address gambler;
        uint32 betSum;
    }
    
    Bet b;
    
    function checkAll() public {
        uint startGas = msg.gas;
        b = Bet(1, 2, 3, 4, 5, 6, 0x112233445566778899aa112233445566778899aa, 21);
        uint diff = startGas - msg.gas;
        assert(diff > 55000);
    }
    
}

[cdetrio]

This might be related to #1137

[greenGreengo]

Yes it's the way the compiler works, it fits in one 32-bytes area but uses 2 sstore.
If you want to want to use just 1 sstore you have to enable --optimize flag when you compile.

$ solc --bin --asm --optimize TestSStore.sol

[androlo]

@cdetrio They are different issues. This is not array related, and technically it is not a bug. At the time of this report, the way storage writes are done (in the case of one-word structs) is:

for all members M in storage struct S:
  read old S from storage to var X
  write M to X
  replace old S with X

Old S is read from storage, modified, and the modified result is written to storage, and this is repeated once for each member. We want it to be:

read old S from storage to var X

for all members M in storage struct S:
  write M to X

replace old S with X

This should also be extended to work for any size struct, which is probably not too difficult since the methods for manipulating packed storage are already in place, and seem to work perfectly fine.

for all storage partitions T of struct S:
  read T from storage to var X

  for all members M that goes into T:
    write M to X

  replace T with X

This should result in one SLOAD and SSTORE per word, rather then one per member variable per word.

[axic]

@androlo that should be fairly safe to implement once storage-related code is written in the IR, but before that it may be a risky change.

[androlo]

@ngotchac

[ngotchac]

I wasn't sure if it was a bug or an enhancement. Seems to be an enhancement then!

[artemii235]

Discovered same using 0.4.19+commit.c4cbbb05.Emscripten.clang, as an example please take a look at this Smart Contract at Ropsten:
https://ropsten.etherscan.io/address/0xa66bc2b55ef5dff147edd7352a633fe8d510839c
It uses Deal struct which contains 7 fields.
When initEthDeal function is called new Deal is created and added to mapping: https://ropsten.etherscan.io/tx/0xb659eb6d17e75fe8e5518398a1d3871c4dece509d43ce38384f3b640422551c9
As we can see by vmtrace (https://ropsten.etherscan.io/vmtrace?txhash=0xb659eb6d17e75fe8e5518398a1d3871c4dece509d43ce38384f3b640422551c9) it causes 7 sstore calls which results in high gas usage for such simple method (over 140k).
In mainnet with gas price = 20 gwei this transaction will cost 3$ (Current ETH price ~1200$) for a caller which is pretty high.
So in my particular case this improvement will help to save a lot money of end users.
My idea is also to pack everything in bytes array and then parse it further. However it would be very nice to have solidity doing it internally without such workarounds.

[artemii235]

My bad, my struct size is much larger than 32 bytes and it's not related to this issue - it's just how EVM works (splitting everything to 32 bytes words and calling SSTORE for each).

[beether]

Not sure if this has yet been noted, but doing this reduces costs to what is expected:

// Compiler is dumb with converting `TestStruct memory` to storage.
// Avoid this.
// s_data = TestStruct({ a: _a, b: _b });

// Compiler is smart when modifying a `TestStruct storage`
// It will write all changes only once (and by 256-bit segment)
s_data.a = _a;
s_data.b = _b;

The optimizer seems smart enough to cache all changes to a TestStruct storage and write them out once. Not sure how it figures out there are no more changes (maybe it writes before any return), but it seems pretty robust -- even if you pass around the TestStruct storage it will still only write once per 256-bit block.

[chriseth]

This might be fixed by #3693 , but we should check.

[chriseth]

This has been fixed.