Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Deterministic HeapHashSet iteration draft #584

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

2 participants