Introduce string map class and switch to it

[asl]

The main change of this PR is a new string_map<V> class that does ordered_map<cstring, V> but in a proper way:

• Key are stored as cstrings in the underlying abseil hash map.
• Heterogenous lookup (with std::string_view keys) is supported. Special care is done not to create cstrings in case if they are not in the map already
• For now map is ordered, values are stored in std::list, similar to ordered_map. Likely we'd need to allow std::vector / absl::inlined_vector as storage as well via dedicated policy, but I need to collect more statistics on this
• Several ordered_map construction bugs were fixed as well

Give this class, I moved two main users of ordered_map<cstring, V> to it:

• IDeclaration
• JSonObject
  trying to refactor & modernize the API to allow & use heterogenous lookup as well.

While there I checked all instances of std::map<cstring, something in frontend / midend and replaced them with unordered maps where appropriate.

While it might look like a breaking change, it is not. Even more, the code eases adopting of downstream code to explicit const char* => cstring conversion, as heterogenous lookup makes these conversions unnecessary. I moved code in backends to this where appropriate.

Fixes #4763

[asl]

For the downstream testcase the compile time changes from:

Executed in   70.79 secs    fish           external
   usr time  107.65 secs    0.05 millis  107.65 secs
   sys time    3.89 secs    1.44 millis    3.89 secs

down to:

Executed in   63.48 secs    fish           external
   usr time   81.99 secs    0.09 millis   81.99 secs
   sys time    3.71 secs    5.54 millis    3.70 secs

So we are having nice 10% speedup. Two things contribute to this change:

• Removal of strcmp in IDeclaration's ordered_map
• Much less malloc traffic there.

[vlstill]

For now map is ordered, values are stored in std::list, similar to ordered_map. Likely we'd need to allow std::vector / absl::inlined_vector as storage as well via dedicated policy, but I need to collect more statistics on this

Or we could just always use std::vector. I doubt std::list will ever be better. Alternatively, if the iteration is rare, some sort of intrusive (singly-) linked list might be usable (I hope noone is reverse iterating over these).

[vlstill]

For now, I went through the data structure implementation. Thank you for your contribution!

[asl]

Or we could just always use std::vector. I doubt std::list will ever be better. Alternatively, if the iteration is rare, some sort of intrusive (singly-) linked list might be usable (I hope noone is reverse iterating over these).

Yes. std::list definitely has much more malloc traffic overhead. And this case is certainly optimized for often erasures. For IDeclaration looks like an intrusive vector would be preferable. For JSonObject – likely the usual std::vector. Anyway, this is something I am planning to measure and play with :)

[vlstill]

Few nitpicks in the map implementation.

[vlstill]

A few comments for the rest of the code. Once we have C++20, we should try to revisit if we can replace the overloads that are the same for cstring and std::string_view with a single one taking a concept that is either. But I am not sure if GCC 9.2 has enough concepts support to allow that or if we would need to wait until we can drop it.

[vlstill]

Should we use the hasher from Utils?

[asl]

It will be used via our std::hash specialization. So, nothing special is needed. Though, abseil on top would salt the hash, so the iteration order will be randomized. I do not have preferences which approach is better.

[asl]

After thinking about this a bit more, maybe we can adopt the following rule:

• For local maps (within the method, etc) we can use Util::Hash directly. This way we won't have additional random salt on top of the existing hash, so the hashing latency will be reduced.
• For members we'd stick with absel's randomized hashes. This way the iteration order will be different on each invocation. As a result, all possible accidentally added iterations could be noticed much earlier.

What do you think?

[asl]

So, I implemented this approach for local maps. Also, found one "funny" case when we had map cstring -> ID which I replaces with set.

[asl]

@vlstill I factored out cstring bits into #4780 to decompose the changes

[ChrisDodd]

For the downstream testcase the compile time changes from:

Executed in   70.79 secs    fish           external
   usr time  107.65 secs    0.05 millis  107.65 secs
   sys time    3.89 secs    1.44 millis    3.89 secs

down to:

Executed in   63.48 secs    fish           external
   usr time   81.99 secs    0.09 millis   81.99 secs
   sys time    3.71 secs    5.54 millis    3.70 secs

So we are having nice 10% speedup. Two things contribute to this change:

• Removal of strcmp in IDeclaration's ordered_map
• Much less malloc traffic there.

How does the performance compare to hvec_map?

[asl]

How does the performance compare to hvec_map?

Its performance was lower in my initial tests. Checked once again to ensure we're comparing apples-to-apples replacing declaration map in IndexedVector with hvec_map<cstring, IDeclaration*>:

Executed in   65.31 secs    fish           external
   usr time  101.39 secs    0.10 millis  101.39 secs
   sys time    1.95 secs    2.28 millis    1.95 secs

[asl]

Rebased after #4780 merged

[asl]

@vlstill @fruffy Anything else left to address?

[fruffy]

LGTM