[RFC] Use Concepts for interface

mratsim commented 6 years ago

Context

As discussed in https://github.com/status-im/nim-eth-common/pull/7, the methods created here are only to describe the interface of AbstractChainDB.

https://github.com/status-im/nim-eth-common/blob/dce32b7d502df64749ca790ad819da1c038b27d2/eth_common/eth_types.nim#L196-L210

AbstractChainDB will not be used for inheritance or heterogeneous collection of objects, hence we could avoid the dynamic dispatch cost.

Usage in Nimbus:

From https://github.com/status-im/nimbus/blob/4944fef3ae33940b26033d1e66c53db2965497a4/nimbus/db/db_chain.nim#L13-L16

type
  BaseChainDB* = ref object of AbstractChainDB
    db*: TrieDatabaseRef
    # TODO db*: JournalDB

Alternative

Instead we could define AbstractChainDB as a concept:

type
  AbstractChainDB = concept x
    x.genesisHash is KeccakHash
    x.getBlockHeader(HashOrNum) is BlockHeaderRef
    x.getBestBlockHeader is BlockHeaderRef
    x.getSuccessorHeader(BlockHeader) is BlockHeaderRef
    x.getBlockBody(KeccakHash) is BlockBodyRef

Potential issues

Getting stuck due to concepts issue like we were for static[T]

zah commented 6 years ago

It's a bit too early to optimize this IMO.

The optimization will be a simple refactoring that can be applied at any point before shipping Nimbus and it's highly unlikely to result in any noticable gains considering that most of these methods are going to invoke some expensive database lookup code that will dwarf the dynamic dispatch costs.

For now, the most important criteria to optimize are the compilation times and the quality of the error messages - both would suffer from switching to concepts right now.

arnetheduck commented 6 years ago

also, fwiu concepts are not like interfaces: they cannot be used in a dynamic setting - what if we want to choose db at runtime based on config or whatever? also, is this really an optimization worth making at all? where are the numbers saying that compared to the db operations, the call indirection causes any significant delay?

mratsim commented 6 years ago

No obviously accessing the db would be more expensive. But when people asked for interfaces in Nim, the answer was that Nim will have something broader and better: concepts, see references below.

At one point they were also supposed to replace methods for runtime polymorphism once VTables were implemented, though that changed and methods are here to stay.

I.e. if we needed to implement interfaces, concepts would be the (future?) Nim idiomatic way, especially now that Araq feels comfortable to promote them in the standard lib https://github.com/nim-lang/Nim/issues/7996. Now if we need them at runtime obviously there is no alternative for methods.

References:

Is there interfaces in NIM language
Should/can we get explicit concepts
https://github.com/nim-lang/Nim/issues/7497 (RFC interface in Nim)
https://github.com/nim-lang/Nim/issues/5770#issuecomment-298886380 (Concepts can express interfaces with implementation-defined types)
https://stackoverflow.com/a/36120073 ("The lack of an explicit interface keyword is common question in the Nim community." ... "If you feel like going for concepts you should ask in the forum directly, but beware, as the manual says, concepts are still in development.")
https://forum.nim-lang.org/t/3128#19699 ("wait until concepts are implemented.", "it was often claimed here that they are of no need for Nim, because concepts are better interfaces")

arnetheduck commented 6 years ago

Thanks for the links - I added https://forum.nim-lang.org/t/4110#25567 as well - looks like I'm with the crowd that considers the syntax odd / sub-par for interfaces, at least compared to what "interface" means in other langs.. if we go with this, as a way of managing expectations, perhaps better to not call them interfaces, because they don't bring the same benefits of simplicity and clarity.

status-im / nim-eth-common