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

sphynx: bump dependency versions #193

Closed
wants to merge 1 commit into from
Closed

sphynx: bump dependency versions #193

wants to merge 1 commit into from

Conversation

jfcg
Copy link
Contributor

@jfcg jfcg commented Aug 12, 2021

Hi,
This change just bumps dependency versions of sphynx. After ./build.sh, there is an error (with/without this change) though:

networkit_wrap.cxx: In function ‘std::vector<double>* _wrap_Centrality_TLX_DEPRECATED_networkit_77eaa497b00f90e1(NetworKit::Centrality*, void*)’:
networkit_wrap.cxx:2001:12: error: ‘arg2’ was not declared in this scope; did you mean ‘arg1’?

I dont know how to fix it :)
Cheers..

@darabos
Copy link
Contributor

darabos commented Aug 12, 2021

Oh, cool, thanks!

I actually have a fix for the NetworKit build issue, it's just in a private repo at the moment. I'll port it back tomorrow along with other build fixes. (I upgraded to Debian Bullseye and it has revealed more than one issue...) Once the main branch is okay I'll check this out!

@darabos
Copy link
Contributor

darabos commented Aug 12, 2021

Wait, you wrote https://github.com/jfcg/sorty! Thanks a lot, it's great! You might find https://github.com/lynxkite/lynxkite/pull/176/files funny. 😄

@jfcg
Copy link
Contributor Author

jfcg commented Aug 12, 2021

I am really happy it is useful in your project :) I have linked to your benchmark in sorty's README. I hope you don't mind it.

To explain #176, there are three different kinds of lesswap calls in sorty. The third kind does not need to do a swap. To simplify things for users of sorty, I wanted to make it possible with just one simple closure. The simplest / cheapest way to realize the third variant was r != s check. In a lesswap (used by sorty) r=s only when they are both zero (any equal values would work to disable swap, zero is just quick to create for CPUs). Also four indices of lesswap is necessary, I tired hard to make it with three but couldn't.

Now why it does not work when you remove the check: Third variant of lesswap may belong to any goroutine working on any part of the range of objects to be sorted. When you remove the check, the lesswap tries to make a (redundant) swap on the zeroth element in the whole range. Zeroth element very likely does not belong to the goroutine that calls the lesswap. As you know, even a 64-bit swap is not guaranteed to be atomic on modern CPUs. The other goroutine that owns the zeroth element maybe writing on it on that very moment, BAM! You have a fancy data race from an innocent removal of r != s;)

Lessons:

  • Even though sorty.Sort has a simple lesswap example in its doc, I need to point out that the form of lesswap is a contract between users and the library.
  • I may need to rethink use of zero for r=s case. It would be just better to use an index from the calling goroutine's range, just in case a user is lazy / smart :P

Now, I am happier that I tried to bump your dependency versions :D, I would have never thought of this possibility.
Cheers..

CC: @olahg @xandrew-lynx

@darabos
Copy link
Contributor

darabos commented Aug 17, 2021

I've moved your commit to #196, where I'm combining it with some build fixes to try to make it pass the tests.

I am really happy it is useful in your project :) I have linked to your benchmark in sorty's README. I hope you don't mind it.

👍

To explain #176, there are three different kinds of lesswap calls in sorty. The third kind does not need to do a swap. To simplify things for users of sorty, I wanted to make it possible with just one simple closure. The simplest / cheapest way to realize the third variant was r != s check. In a lesswap (used by sorty) r=s only when they are both zero (any equal values would work to disable swap, zero is just quick to create for CPUs). Also four indices of lesswap is necessary, I tired hard to make it with three but couldn't.

I guess the alternative would be a separate less function, right? But who wants to write two functions for a sort call.

Thanks for the detailed explanation!

  • Even though sorty.Sort has a simple lesswap example in its doc, I need to point out that the form of lesswap is a contract between users and the library.

Perfect! Thanks for adding that!

  • I may need to rethink use of zero for r=s case. It would be just better to use an index from the calling goroutine's range, just in case a user is lazy / smart :P

Oh this is a nice fix too! It almost looks safe now to skip the check. 😄

@darabos darabos closed this Aug 17, 2021
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.

None yet

2 participants