Rewrite the selector table in C++.

gnustep / libobjc2

Objective-C runtime library intended for use with Clang.

http://www.gnustep.org/

MIT License

426 stars 116 forks source link

Rewrite the selector table in C++. #241

Closed davidchisnall closed 1 year ago

davidchisnall commented 1 year ago

This replaces a few home-grown datastructures with third-party ones that get a lot more testing:

The home-grown hopscotch hash table is moved to using robin map. The original was designed to be lock free, but we've been using it behind a lock for ages.
The selector list is now a std::vector.
The types list now use std::forward_list.

This also removes a couple of code paths that haven't been used since we started using the new ABI data structures internally and upgrading at load time.

On a single test machine (not guaranteed to be representative) the test suite now completes around 20% faster.

davidchisnall commented 1 year ago

Well, this seems to crash everywhere except FreeBSD. That's annoying...

davidchisnall commented 1 year ago

The Windows failure was a misplaced #endif, the Ubuntu failures seem to be real bugs.

davidchisnall commented 1 year ago

It looks as if this still has an intermittent failure in the ManyManySelectors test. This suggests that the reason for the original test failure was not the hash table: the new one fails in the same way, intermittently returning the previous selector value for the new one, yet the underlying data structures are now completely different.

davidchisnall commented 1 year ago

Note that the test is repeatedly registering selectors with empty types, so the only things that should be able to trigger the failure are the hash and equality on the names, but these are simple C-string comparisons.

davidchisnall commented 1 year ago

I suspect that the failure was due to the isSelRegistered function spuriously returning the wrong value. I've now separated out the registered and unregistered selector types as much as possible in the static type system so that almost every code path is able to skip the check. There's probably a bit more to do, but almost all of the accesses are now either very early on (where selectors would have to be on the null page to have problems) or in legacy ABI compat things.

triplef commented 1 year ago

Thanks, looks like a nice improvement! It seems to work well for our app, although I didn’t see any noticeable performance improvements.