Make dict and pool const lookup methods never rehash the hashtable

[rocallahan]

What are the reasons/motivation for this change?

Casting away const is dirty and it's pretty easy to avoid in this case.
It's also probably a tiny bit faster to avoid rehashes if we're only doing lookups.

Explain how this is achieved.

Just refactor the code a little.

[widlarizer]

Method constness in this codebase is in a couple places used to label operations that don't modify information actually conveyed and that retain equivalence under operator=. An alternative in some cases is to have mutable members, but I don't see it as practical for dict. The existence of the C++ mutable keyword signifies that constness being a separate concept from a constness a compiler might consider, for example. This makes casting away const less "dirty" and more like "idiomatic C++".

The rehashing is behind a branch, so if the rehashing never actually occurs, I would expect this check to be free in the world of modern branch prediction and out of order computing. Unless there is tangible perf benefit to this code change, I don't think it's worth touching this as it's at the core of yosys

[rocallahan]

This makes casting away const less "dirty" and more like "idiomatic C++".

I argue that casting away const is idiomatic C++ when it's necessary, and here it isn't. const doesn't always mean "immutable", but mutation-under-the-hood is always a little bit surprising. I agree that this is subjective.

The rehashing is behind a branch, so if the rehashing never actually occurs,

It does occur. A call to insert() calls do_lookup() which rehashes if the entries.size() before insertion exceeds the threshold, and then appends the element to entries. A following call to find()/at()/count() can observe that entries.size() after insertion exceeds the threshold and thus rehash.

Unless there is tangible perf benefit to this code change, I don't think it's worth touching this as it's at the core of yosys

Yeah, I can't argue that the perf benefit is "tangible" and I appreciate that touching core code requires care. But while the benefit may be very modest, this is an extremely simple change that is easy to verify by eye.

[rocallahan]

mutation-under-the-hood is always a little bit surprising

The other thing at the back of my mind is that although Yosys is fully single-threaded today, there are potentially huge performance benefits from parallelism. Obviously lots of issues block that, but making "const' methods actually non-mutating, where that's easy to do, is a step in the right direction.

If the firm intent of the Yosys teams is that Yosys will never support parallelism, not even read-only access to RTLIL from multiple threads, just let me know and I'll stop thinking about it :-).