search

celestiaorg / smt

A Go library that implements a Sparse Merkle tree for a key-value map.
https://godoc.org/github.com/celestiaorg/smt
MIT License
138 stars 53 forks source link

Offset calculated on every iteration #34

Closed davebryson closed 3 years ago

davebryson commented 3 years ago

Just noticed this while working through the code. Does this need to be calculated on every iteration?

https://github.com/lazyledger/smt/blob/master/smt.go#L289

When I move it above the for loop, all test still pass.

adlerjohn commented 3 years ago

Permalink:

https://github.com/lazyledger/smt/blob/54602cb54f9753ea062755e046003b73b47922d0/smt.go#L289

The Go compile is smart enough to perform loop invariant code motion. I don't know if it does in this case, but it's not a guarantee that performing the optimization by hand would actually be doing anything that isn't already being done by the compiler. The generated assembly would have to be analyzed to determine if the compiler is performing the optimization or not.

liamsi commented 3 years ago

@davebryson @adlerjohn does it make sense to analyze the generated assembly in this case? Also, is there anything else that needs further clarification? Otherwise, I'd close this issue.

adlerjohn commented 3 years ago

I can implement the change, since it's trivial and doesn't hurt.

liamsi commented 3 years ago

Yeah, looking at the line in question: independent from how the go compiler handles this, I think it makes sense to move this for readability (to save others the cognitive overhead and time thinking about this).