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

Remove erroneous accumulated data addition during merge! #129

Merged
merged 1 commit into from
Jun 26, 2021

Conversation

IanButterworth
Copy link
Contributor

This appears to be a design error in #128

Although I don't have a MWE, in real world usage in a threading setup with v0.5.10 I was seeing parents with 0 calls, time & allocations, which is fixed by removing this line.

@KristofferC
Copy link
Owner

Hmm, so what did this line actually do? Do we lack in test coverage since the addition / removal of this line doesn't change the test status.

@IanButterworth
Copy link
Contributor Author

I don't understand why it didn't result in the top parent having ncalls equal to the totality of the children in the path to the merge point in the tests.

I suspect it's that the accumulated data is being added before it's concluded, so a threading issue, but perhaps shouldn't be done anyway?

What do you think the right thing to do here is?

I can try to get a test that replicates the issue

@IanButterworth
Copy link
Contributor Author

Hmm, so what did this line actually do?

Specifically it was supposed to accumulate what happens on threads into the parents of the merge point.

As was motivated by the original PR, the main reason someone would want a custom merge point would be to cover threaded sections.

Looking at what @time reports

julia> @time Threads.@spawn @time begin sleep(1); x = rand(10000); end;
  0.001388 seconds (45 allocations: 3.372 KiB)
  1.005833 seconds (6 allocations: 78.328 KiB)

What happens on the thread isn't accumulated onto the top level report, and at each item it just reports how quickly calls return.

So I think it was bad design and a bit of an anti-pattern, and this PR fixes that.

@KristofferC KristofferC merged commit b5fd7ad into KristofferC:master Jun 26, 2021
@IanButterworth IanButterworth deleted the ib/custom_merge_fix branch June 26, 2021 19:55
@IanButterworth
Copy link
Contributor Author

@KristofferC it'd be good to get a release with this. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants