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

Reduce the extra overhead caused by greedy equal_unions #48410

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Jan 26, 2023

It turns out that #48221 increases the overhead of subtyping if equal_unions return false.
The failing equal_unions would be re-subtyped during the next exists_subtype.
This PR tries to avoid that by:

  1. First exhausting equal_unions branch and setting state[i] = 1 if it failed.
  2. Skipping the checked part when we perform the next equal_unions.

Some simple benchmark with subtype test itself as the reference

Test  (Worker) | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
subtype    (1) |    12.97 |   0.49 |  3.8 |    1919.17 |   680.35  # master
subtype    (1) |    12.71 |   0.43 |  3.4 |    1843.41 |   685.72  # with 1
subtype    (1) |    12.19 |   0.47 |  3.8 |    1660.20 |   688.86  # with 1 + 2

@N5N3 N5N3 added performance Must go faster domain:types and dispatch Types, subtyping and method dispatch labels Jan 26, 2023
@N5N3 N5N3 force-pushed the skip_eq_union branch 3 times, most recently from 15a6be7 to 9b58ccb Compare January 26, 2023 07:18
@N5N3
Copy link
Member Author

N5N3 commented Jan 26, 2023

@nanosoldier runtests(["ProxAL"])

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected.
A full report can be found here.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 31, 2023

The cleanup commit LGTM. I think we could make that a separate PR and merge that immediately.

The first idea ("Avoid repeated failing equal_union.") seems logical to me. If I understand it correctly, this is just a change to the heuristic search order (whether we consider this to be exhausting the Lunions or Runions). And since we were already treating this as an Runion branch, it was not necessary to have the fall-through happen immediately, as we are already guaranteed to revisit that later if needed.

The second one seems less certain to me (meaning mostly that i am still trying to understand it better). I guess there we will then be eagerly exploring all of the Runion choices starting from this point. Why do we not doing that always when handling an Runion? Should we do that always for Runions (when !sub) and Lunions (when sub)?

@N5N3
Copy link
Member Author

N5N3 commented Feb 1, 2023

I guess there we will then be eagerly exploring all of the Runion choices starting from this point.

Yes. Based on in mind simulation, this change will not cause behavior change.
The next exists_subtype will repeat the previous subtyping and hit the same branch, only a deeper decision will be reversed.

Why do we not doing that always when handling an Runion.

I thought excessive save/restore env is not ideal, thus only tried it in this hot path.
But yes, it might be better to apply this in subtype.
We can set a save point if Runions.depth % N = 0, and the overhead of GC and re-subtyping could be balanced with a tuned N

and Lunions (when sub)?

Lunions is another story as the env save point doesn't make sense there. But I tried to make forall_exists_equal test all local Lunions in #48441.
It accelerated large union equal test and test added in #48221 passed even with equal_union path removed.

@N5N3
Copy link
Member Author

N5N3 commented Feb 1, 2023

Looks like the speed up of save point added in this PR could be ignored after #48441.
As a localized >: check in forall_exists_equal reduce the cost of <: dramatically. (especially if both X and Y are complex Union when we test X == Y)
Which also means we need a better benchmark to test the save point idea.
So I think we can finish this PR with the 1st change only.

@N5N3 N5N3 force-pushed the skip_eq_union branch 2 times, most recently from 5e8a4cb to bead5c3 Compare February 1, 2023 04:37
We'd better skip checked `equal_union` by set `state[i] = 1`
@vtjnash vtjnash added the needs pkgeval Tests for all registered packages should be run with this change label Feb 1, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 1, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@N5N3
Copy link
Member Author

N5N3 commented Feb 1, 2023

@nanosoldier runtests(["FastaLoader", "PyBraket", "IntervalTrees", "CORBITS", "EasyModelAnalysis", "FrameFun", "RoME"])

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@N5N3 N5N3 removed the needs pkgeval Tests for all registered packages should be run with this change label Feb 1, 2023
@N5N3 N5N3 merged commit c18909d into JuliaLang:master Feb 1, 2023
@N5N3 N5N3 deleted the skip_eq_union branch February 1, 2023 14:13
N5N3 added a commit to N5N3/julia that referenced this pull request Mar 17, 2023
skip checked `equal_union` by set `state[i] = 1`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:types and dispatch Types, subtyping and method dispatch performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants