Skip to content

Conversation

@sgeisenh
Copy link

Work in progress toward getting all uses of HeapHashSet iteration to have stable ordering between record and replay.

There's a lot to be improved here but it mostly demonstrates what kind of changes might be required to get more coverage using compile-time techniques.

After working through these changes over the past few days, I think @oridb's upcoming container changes (iteration order based on insertion order) might be a more maintainable path toward consistent ordering, in general.

Some of the challenges with the compile-time techniques:

  • Forward declarations are pervasive in the Chromium codebase. This makes it difficult to understand where cyclic dependencies may potentially exist between types.
  • When a cyclic dependency does exist, I don't think there's a better solution than manually specifying the hash type to use.
  • Adding a static_assert to HeapHashSet::begin was the most reliable way that I found to easily identify forward declaration issues. But there's no good way to disambiguate between types that are actually missing a RecordReplayId method and incomplete types that already have a RecordReplayId method.
  • The use of recordreplay::RecordReplayIdMixin across a large number of header files does significantly increase compile times after changing base/record_replay.h.

@Domiii
Copy link

Domiii commented Apr 21, 2023

As we have just discussed: It might be less intrusive/worth investigating to use PointerId and use MakeGarbageCollected and friends to register/unregister them instead?

@sgeisenh
Copy link
Author

I tried a couple of approaches to getting pointer registration working on garbage collected objects but haven't been able to get much of a handle on destruction behavior during GC sweep. The first was to try unregistering pointers during pre-finalization and the second was to try to add a destructor implementation to cppgc::GarbageCollected. I didn't get very far with either before fighting with the compiler. There's probably something clever that could be done here, but I'd definitely have to dig deeper into what's going on. At one point in oilpan's development, there were two garbage collected base classes (GarbageCollected and GarbageCollectedFinalized) but I think they replaced the latter with some type trait magic (I found this 2015 presentation on oilpan: https://docs.google.com/presentation/d/1XPu03ymz8W295mCftEC9KshH9Icxfq81YwIJQzQrvxo/htmlpresent).

It's a bit frustrating because it feels like it shouldn't be too hard to get something to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants