Skip to content

C++: Fix join order in def-by-reference data flow#1070

Merged
geoffw0 merged 1 commit into
github:rc/1.20from
jbj:dataflow-defbyref-join-order
Mar 12, 2019
Merged

C++: Fix join order in def-by-reference data flow#1070
geoffw0 merged 1 commit into
github:rc/1.20from
jbj:dataflow-defbyref-join-order

Conversation

@jbj

@jbj jbj commented Mar 11, 2019

Copy link
Copy Markdown
Contributor

This PR fixes a regression introduced in #1000.

The performance was adequate on most projects but degenerated on https://github.com/Microsoft/Tocino.

The performance was adequate on most projects but degenerated on
https://github.com/Microsoft/Tocino.
@jbj jbj added the C++ label Mar 11, 2019
@jbj jbj added this to the 1.20 milestone Mar 11, 2019
@jbj jbj requested a review from a team as a code owner March 11, 2019 10:05
@jbj

jbj commented Mar 11, 2019

Copy link
Copy Markdown
Contributor Author

This also affects nsnam/ns-3-dev-git.

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks right to me. How confident are you of performance?

@jbj

jbj commented Mar 12, 2019

Copy link
Copy Markdown
Contributor Author

From a practical point of view, I've tested the change on the two problematic projects, and the performance issue went away. From a theoretical point of view, the or ... section that's added can't contribute more tuples than the number of definitions by reference in the program times the average number of CFG predecessors they have (should be very close to one).

@geoffw0

geoffw0 commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

OK, I'll give it a quick spin myself (this is for the RC branch after all) and then merge.

@geoffw0

geoffw0 commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

Seems fine. Merging.

@geoffw0 geoffw0 merged commit 77c983b into github:rc/1.20 Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants