Skip to content

Conversation

@JeffBezanson
Copy link
Member

This is a refactoring that should be NFC. I changed the code to loop over union-split signatures in an outer loop, and handle the cases uniformly whether they come from union splitting or just multiple matches.

There are two main reasons for this: (1) it should be faster, since now it makes the same _methods_by_ftype queries that inference does (inference splits unions before doing those lookups). (2) it makes a bit more sense to me and the code feels less repetitive --- e.g. now there is only one call to analyze_method! instead of two. I think this will make it easier to adjust the logic for when we decide to convert dispatch to branches.

A couple numbers. Before:

Base  ─────────── 39.296041 seconds
Stdlibs: ────  56.155352 seconds 58.8313%
TTFP: 13.783342 seconds (15.35 M allocations: 808.146 MiB, 3.25% gc time)

after:

Base  ─────────── 38.485334 seconds
Stdlibs: ────  54.897741 seconds 58.7876%
TTFP: 13.533637 seconds (15.12 M allocations: 800.206 MiB, 3.08% gc time)

@JeffBezanson JeffBezanson added compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) latency Latency labels May 14, 2020
@JeffBezanson
Copy link
Member Author

@Keno @vtjnash Ok with this? I suddenly really want to merge it, since it allows existing union-splitting cases to remain fast even if we set max_methods to 3.

@vtjnash
Copy link
Member

vtjnash commented Jun 17, 2020

Sounds reasonable to me. Makes we wonder if we should make this a shared helper, but I currently suspect not.

@JeffBezanson JeffBezanson merged commit 8ca6e8d into master Jun 17, 2020
@JeffBezanson JeffBezanson deleted the jb/inliningrefactor branch June 17, 2020 19:41
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) latency Latency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants