Stream: t-compiler/wg-incr-comp

Topic: Order dependence of `Task


view this post on Zulip Aaron Hill (Jun 06 2021 at 22:22):

I've been experimenting locally with removing TaskDeps.reads, and just using TaskDeps.read_set. However, it appears that something is relying on the insertion order being preserved. If I just rely on the iteration order of the FxHashSet (where we previously iterated over TaskDeps.reads, then two incremental tests fail. If I switch it to use linked_hash_set::LinkedHashSet, then the tests pass

view this post on Zulip Aaron Hill (Jun 06 2021 at 22:24):

the insertion order is itself pretty arbitrary (whatever order the query access its dependencies in), so I think that the problem arises from the order changing. The failing tests both fail to recover a DefId, which suggests that we're incorrectly marking a query as green (e.g. we are somehow losing track of edges). However, I don't see anything in the code that relies on the order of edges being unchanged

view this post on Zulip Aaron Hill (Jun 07 2021 at 03:54):

Never mind - nothing was getting marked green incorrectly. Since the edges were in a different order during loading, marking the node green tried to load a different query before the tcx.hir() dependency. When the edges are loaded in order, the 'parent' dependency (e.g. hir().local_def_id_to_hir_id will always be run first, which prevents this problem)

view this post on Zulip cjgillot (Jun 07 2021 at 16:42):

Can you elaborate on your reasoning? I am not sure I follow.
Neither tcx.hir() nor tcx.hir().local_def_id_to_hir_id are queries.

view this post on Zulip Aaron Hill (Jun 07 2021 at 19:51):

I didn't look to closely into it - the failing query was lookup_deprecation_entry

view this post on Zulip Aaron Hill (Jun 07 2021 at 19:53):

I see - I forgot that tcx.hir() doesn't actually invoke any queries

view this post on Zulip Aaron Hill (Jun 07 2021 at 19:54):

what I think happened was something like let res = tcx.first_query(val); tcx.second_query(res) inside of a query impl

view this post on Zulip Aaron Hill (Jun 07 2021 at 19:54):

when we tried to mark the deps of that query as green, we started with second_query

view this post on Zulip Aaron Hill (Jun 07 2021 at 19:55):

normally, we would have started with first_query, which would have failed, so we would never get to second_query

view this post on Zulip Aaron Hill (Jun 07 2021 at 20:13):

This could probably be reproduced without any of the hash set changes by inserting tasl+deps.reads.reverse() in the proper place

view this post on Zulip mw (Jun 08 2021 at 15:59):

Preserving the order of dependencies is needed for correctness. A query provider is still just a regular procedure. I don't have time right now but I'll try to come up with a good example tomorrow.


Last updated: Oct 21 2021 at 20:03 UTC