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

massive speed up for ipu #31

Merged
merged 1 commit into from Jan 15, 2017

Conversation

Projects
None yet
4 participants
@Eh2406
Contributor

Eh2406 commented Dec 28, 2016

on our data the old code synthesized a bg in 70-80 sec with this change it does it in 2-3 sec, more than 20 times faster.

cc @semcogli

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Jan 3, 2017

Contributor

Very cool! Looking forward to checking this out. Like you mentioned in the other pull request, the failing Travis runs are due to the separate issue of deprecated index.diff

Contributor

janowicz commented Jan 3, 2017

Very cool! Looking forward to checking this out. Like you mentioned in the other pull request, the failing Travis runs are due to the separate issue of deprecated index.diff

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 3, 2017

Contributor

It sounds like we need a version of UDST/urbansim#179. Then I can rebase on top of that. How would you like me to proceed?

Contributor

Eh2406 commented Jan 3, 2017

It sounds like we need a version of UDST/urbansim#179. Then I can rebase on top of that. How would you like me to proceed?

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Jan 3, 2017

Contributor

Yes, good call. Thanks. We'll get a version of urbansim#179 in today, and then notify you so you can rebase.

Contributor

janowicz commented Jan 3, 2017

Yes, good call. Thanks. We'll get a version of urbansim#179 in today, and then notify you so you can rebase.

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Jan 3, 2017

Contributor

@Eh2406 The deprecated index.diff has been updated now, and the change is in master:
#32

Can you rebase the pull requests when you get the chance?

Contributor

janowicz commented Jan 3, 2017

@Eh2406 The deprecated index.diff has been updated now, and the change is in master:
#32

Can you rebase the pull requests when you get the chance?

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 3, 2017

Contributor

I have a meeting, I will rebase when it is done. :-) Thanks.

Contributor

Eh2406 commented Jan 3, 2017

I have a meeting, I will rebase when it is done. :-) Thanks.

@Eh2406

This comment has been minimized.

Show comment
Hide comment
@Eh2406

Eh2406 Jan 3, 2017

Contributor

@semcogli is also working on making sure it is fast on other work loads, not just ours.

Contributor

Eh2406 commented Jan 3, 2017

@semcogli is also working on making sure it is fast on other work loads, not just ours.

massive speed up for ipu
on our data the old code synthesized a bg in 70-80 sec with this change it does it in 2-3 sec, more than 20 times faster.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 3, 2017

Coverage Status

Coverage increased (+0.04%) to 81.296% when pulling f66fd78 on SEMCOG:speed_up_drop_deros into 6d2f3dc on UDST:master.

coveralls commented Jan 3, 2017

Coverage Status

Coverage increased (+0.04%) to 81.296% when pulling f66fd78 on SEMCOG:speed_up_drop_deros into 6d2f3dc on UDST:master.

@semcogli

This comment has been minimized.

Show comment
Hide comment
@semcogli

semcogli Jan 3, 2017

A little more info about this update. If the table size is not big, say less than 2000 columns, the original code actually works faster. However, the frequency table we are dealing here has 32257 columns. In this case, the new code will use much shorter time.

semcogli commented Jan 3, 2017

A little more info about this update. If the table size is not big, say less than 2000 columns, the original code actually works faster. However, the frequency table we are dealing here has 32257 columns. In this case, the new code will use much shorter time.

@janowicz

This comment has been minimized.

Show comment
Hide comment
@janowicz

janowicz Jan 9, 2017

Contributor

Looks good to me. I tried synthesis test runs with a couple different recipes, before and after the change above- and observed a speed-up when using this change in both cases, with the greater speedup coming in the recipe that results in bigger frequency table (in line with the comment above). I think this trade-off is ok, as I think most operational workloads will tend towards synthesizing with more constraints.

Contributor

janowicz commented Jan 9, 2017

Looks good to me. I tried synthesis test runs with a couple different recipes, before and after the change above- and observed a speed-up when using this change in both cases, with the greater speedup coming in the recipe that results in bigger frequency table (in line with the comment above). I think this trade-off is ok, as I think most operational workloads will tend towards synthesizing with more constraints.

@janowicz janowicz merged commit 7bd1b84 into UDST:master Jan 15, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 81.296%
Details

@Eh2406 Eh2406 deleted the SEMCOG:speed_up_drop_deros branch Jan 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment