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

[hail] Improve split_multi_hts performance #6980

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@tpoterba
Copy link
Collaborator

commented Sep 3, 2019

No description provided.

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

master:

2019-09-03 07:04:50,033: INFO: [1/1] Running split_multi_hts...
2019-09-03 07:05:54,871: INFO:     burn in: 64.79s
2019-09-03 07:07:00,623: INFO:     run 1: 65.74s
2019-09-03 07:08:05,181: INFO:     run 2: 64.56s
2019-09-03 07:09:07,775: INFO:     run 3: 62.59s

PR:

2019-09-03 07:10:34,045: INFO: [1/1] Running split_multi_hts...
2019-09-03 07:11:16,374: INFO:     burn in: 42.33s
2019-09-03 07:11:56,085: INFO:     run 1: 39.71s
2019-09-03 07:12:34,916: INFO:     run 2: 38.83s
2019-09-03 07:13:14,087: INFO:     run 3: 39.17s
@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

This is almost all coming from the Python change. the gqFromPL change has a negligible effect on performance, but still seems good to include.

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

more improvements (though less simple) to come.

tpoterba added a commit to tpoterba/hail that referenced this pull request Sep 3, 2019
[hail] Introduce `ArrayFold2` to permit more deforesting
Problem 1: functions like `min`/`max` need to do a length check,
preventing us from implementing them in a single primitive fold.

Problem 2: functions like `mean` use  a struct as the accumulator,
leading to allocation (!) every element.

Solution to both: make it possible to have multiple primitive
accumulators. This is `ArrayFold2`.

The node is different from `ArrayFold` in that it:
 * has as sequence of accumulators, not just one
 * has a sequence of seq ops, one for each accumulator. Each of these
   sequence ops can see all the accumulators, and will see the updated
   value from sequence operations with a smaller index.
 * has a result op, which is a function from accumulators to result.

By changing `min`/`max` to use ArrayFold2 and inlining these functions,
we can get a reasonable speedup on `split_multi_hts`:

hail-is#6980 (this PR's parent):

```
2019-09-03 07:11:16,374: INFO:     burn in: 42.33s
2019-09-03 07:11:56,085: INFO:     run 1: 39.71s
2019-09-03 07:12:34,916: INFO:     run 2: 38.83s
2019-09-03 07:13:14,087: INFO:     run 3: 39.17s
```

PR:
```
2019-09-03 07:32:10,416: INFO:     burn in: 38.03s
2019-09-03 07:32:39,237: INFO:     run 1: 28.82s
2019-09-03 07:33:07,778: INFO:     run 2: 28.50s
2019-09-03 07:33:35,997: INFO:     run 3: 28.21s
```
@@ -27,7 +27,7 @@ object GenotypeFunctions extends RegistryFunctions {
tPL.isElementDefined(region, pl, i).mux(
Code._empty,
Code._fatal("PL cannot have missing elements.")),
pli := region.loadInt(tPL.loadElement(region, pl, i)),
pli := region.loadInt(tPL.loadElement(region, pl, len, i)),

This comment has been minimized.

Copy link
@jigold

jigold Sep 3, 2019

Collaborator

Why were you able to change the signature of loadElement? len doesn't look like it's used anywhere

This comment has been minimized.

Copy link
@tpoterba

tpoterba Sep 3, 2019

Author Collaborator

the signature without len will load the length to calculate how many missing bits there are, and therefore where the elements start. This signature is faster.

if 'GQ' in entry_fields:
update_entries_expression['GQ'] = hl.or_else(hl.gq_from_pl(pl), split.GQ)
pl_gq_struct = hl.rbind(pl, lambda pl: hl.struct(PL=pl, GQ=hl.gq_from_pl(pl)))
update_entries_expression['PL'] = pl_gq_struct['PL']

This comment has been minimized.

Copy link
@jigold

jigold Sep 3, 2019

Collaborator

Is there a reason you added update_entries_expression['PL'] here?

This comment has been minimized.

Copy link
@tpoterba

tpoterba Sep 3, 2019

Author Collaborator

sorry, don't fully understand the question.

This change relies on the deduplication logic added to StructExpression.annotate that tries to figure out if a struct was exploded annotate(**thing) and pulls thing out in a Let.

The change will bind the pl_gq_struct in a Let if both appear.

addressed

@jigold
jigold approved these changes Sep 3, 2019

@danking danking merged commit 69ba846 into hail-is:master Sep 3, 2019

1 check passed

ci-test success
Details
tpoterba added a commit to tpoterba/hail that referenced this pull request Sep 3, 2019
[hail] Introduce `ArrayFold2` to permit more deforesting
Problem 1: functions like `min`/`max` need to do a length check,
preventing us from implementing them in a single primitive fold.

Problem 2: functions like `mean` use  a struct as the accumulator,
leading to allocation (!) every element.

Solution to both: make it possible to have multiple primitive
accumulators. This is `ArrayFold2`.

The node is different from `ArrayFold` in that it:
 * has as sequence of accumulators, not just one
 * has a sequence of seq ops, one for each accumulator. Each of these
   sequence ops can see all the accumulators, and will see the updated
   value from sequence operations with a smaller index.
 * has a result op, which is a function from accumulators to result.

By changing `min`/`max` to use ArrayFold2 and inlining these functions,
we can get a reasonable speedup on `split_multi_hts`:

hail-is#6980 (this PR's parent):

```
2019-09-03 07:11:16,374: INFO:     burn in: 42.33s
2019-09-03 07:11:56,085: INFO:     run 1: 39.71s
2019-09-03 07:12:34,916: INFO:     run 2: 38.83s
2019-09-03 07:13:14,087: INFO:     run 3: 39.17s
```

PR:
```
2019-09-03 07:32:10,416: INFO:     burn in: 38.03s
2019-09-03 07:32:39,237: INFO:     run 1: 28.82s
2019-09-03 07:33:07,778: INFO:     run 2: 28.50s
2019-09-03 07:33:35,997: INFO:     run 3: 28.21s
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.