Fix assertion failure from erroneous unlock in Adaptive OBIM. #350

insertinterestingnamehere · 2020-07-23T03:17:32Z

Fixes #349, though I'm not entirely sure why yet.

bozhiyou · 2020-07-26T16:16:12Z

The problem is actually not to do with the lock ownership. See the inline comments.

bozhiyou · 2020-07-26T16:20:51Z

libgalois/include/galois/worklists/AdaptiveObim.h

@@ -296,8 +296,7 @@ struct AdaptiveOrderedByIntegerMetric : private boost::noncopyable {
        ((double)(p.slowPopsLastPeriod) / (double)(p.sinceLastFix)) >
            1.0 / (double)(chunk_size)) {
      for (unsigned i = 1; i < runtime::activeThreads; ++i) {
-        while (current.getRemote(i)->lock.try_lock())


Here it is the culprit - something is missing!

Great find. That totally explains why it was broken before. Thanks!

bozhiyou · 2020-07-26T16:21:18Z

libgalois/include/galois/worklists/AdaptiveObim.h

@@ -358,8 +357,7 @@ struct AdaptiveOrderedByIntegerMetric : private boost::noncopyable {
          delta = 0;

        for (unsigned i = 1; i < runtime::activeThreads; ++i) {
-          while (current.getRemote(i)->lock.try_lock())


bozhiyou · 2020-07-26T16:26:51Z

libgalois/include/galois/worklists/AdaptiveObim.h

@@ -473,8 +471,7 @@ struct AdaptiveOrderedByIntegerMetric : private boost::noncopyable {

  void push(const value_type& val) {
    ThreadData& p = *current.getLocal();
-    while (!p.lock.try_lock())


The explicit lock here is fine. I prefer to start a new issue/pr for replacing some explicit lock with std::lock_guard if necessary.

bozhiyou

A real issue is that numPushesThisStep can be 0, not to be divided.

… of OBIM.

insertinterestingnamehere · 2020-07-27T15:12:12Z

libgalois/include/galois/worklists/AdaptiveObim.h

+    : private boost::noncopyable,
+      public internal::AdaptiveOrderedByIntegerMetricComparator<Index,
+                                                                UseDescending> {
+  template <typename _T>


I actually moved the underscore to after the names deliberately since underscore prefixes followed by a capital letter are reserved symbols for the compiler. That's an issue that's present throughout the current codebase though, so we can just address it here later along with everything else.

That's a good point. I was just making it resemble OBIM's code without criticism on the old OBIM code. I should have discussed it with you. Thanks!

insertinterestingnamehere · 2020-07-27T15:39:14Z

libgalois/include/galois/worklists/AdaptiveObim.h

      for (unsigned i = 1; i < runtime::activeThreads; ++i) {
-        while (current.getRemote(i)->lock.try_lock())
+        while (!data.getRemote(i)->lock.try_lock())


Can we just do an explicit call to lock instead of while loops? It seems simpler and may be slightly better for performance.

Actually, I'm not quite aware of the difference between try_lock and lock - Idk why they were designed separately.

I think it's a performance thing. I'm not sure about the details involved though.

insertinterestingnamehere · 2020-07-27T16:06:02Z

libgalois/include/galois/worklists/AdaptiveObim.h

      }
    }
-#ifdef UNMERGE_ENABLED


This block was originally disabled. Did you mean to re-enable it? I assume they disabled it for performance reasons.

I made this macro-control a template parameter, which is still false/disabled by default. To me, it doesn't make sense to keep a statically disabled code block in the codebase.

Awesome. I missed that change. That's a great idea!

insertinterestingnamehere · 2020-07-27T16:09:37Z

libgalois/include/galois/worklists/AdaptiveObim.h

+namespace internal {
+
+template <typename Index, bool UseDescending>
+struct AdaptiveOrderedByIntegerMetricComparator {


When porting the code to the latest version I actually set things up to remove any reliance on the OBIM-style internal comparator classes since they really only exist to manage the direction the comparisons run. It's not a huge deal one way or the other, but I'd ultimately like to move OBIM and Adaptive OBIM away from using these helper classes since they're very verbose for what they actually are doing.

That makes sense. Again, I was matching the code structure with OBIM assuming that OBIM's code was in a good shape. Now OBIM should be the one to change.

Yah, these are really easy to change. I can just take care of it in a separate PR after we get this one sorted out.

insertinterestingnamehere · 2020-07-27T16:13:15Z

libgalois/include/galois/worklists/AdaptiveObim.h

@@ -296,8 +296,7 @@ struct AdaptiveOrderedByIntegerMetric : private boost::noncopyable {
        ((double)(p.slowPopsLastPeriod) / (double)(p.sinceLastFix)) >
            1.0 / (double)(chunk_size)) {
      for (unsigned i = 1; i < runtime::activeThreads; ++i) {
-        while (current.getRemote(i)->lock.try_lock())


Great find. That totally explains why it was broken before. Thanks!

libgalois/include/galois/worklists/AdaptiveObim.h

bozhiyou

I've fixed as many issues with ill-logic as I could so far. Other changes are just to conform with the old OBIM code assuming that OBIM is well-implemented. I prefer to make the code uniform and even considered making AdaptiveOBIM a derived class of OBIM since there is a bunch of repeated code.

Although this doesn't affect our current project, I prefer to somehow fix it now rather than revisit this later - or never, we don't know when. It's handy and does not take too much time.

insertinterestingnamehere · 2020-07-27T22:06:26Z

Okay, is this good to go now? It seems like the immediate outstanding issues for this PR have been taken care of.

bozhiyou · 2020-07-27T22:15:12Z

Okay, is this good to go now? It seems like the immediate outstanding issues for this PR have been taken care of.

I think so given that the implementation details w.r.t. OBIM will be handled in another PR. Will merge then. Please also test it on A* or whatever since I only test it with sssp to make sure nothing's error prone.

insertinterestingnamehere · 2020-07-27T22:15:56Z

WRT subclassing, that's an interesting question. It seems like something that could maybe work, but I'm not sure whether or not we'd end up with a net simplification. For example, the UniformBSP parameter doesn't really make sense for the adaptive OBIM scheduler.

If this is in a working state, I'd like to postpone further modifications/simplifications until after the upcoming paper deadline. We've got a lot left to do for that.

insertinterestingnamehere · 2020-07-27T22:17:34Z

Yah, I've just been testing it on SSSP and FMM too. The A* code needs to be forward ported like this was, so I'll tackle it after a few deadlines have passed.

insertinterestingnamehere · 2020-07-27T23:55:45Z

I was just testing out the new version and now it's deadlocking in SSSP. Any idea why?

bozhiyou · 2020-07-28T00:09:34Z

Works fine here. Could you please provide the command line?

A known issue is that gcc complains somewhere using '*' as the multiplication operator in boolean expressions which by default is dereferencing. Will commit a patch soon.

bozhiyou · 2020-07-28T00:43:04Z

Just pushed a patch to the repo on the cdgc server but it seems not sync'ed here.

insertinterestingnamehere · 2020-07-28T00:58:37Z

Yah, it's not automatically synchronized. If you push something to one place you should also push to the other.

insertinterestingnamehere · 2020-07-28T00:59:00Z

#351 is the concrete test case I'm using.

insertinterestingnamehere · 2020-07-28T01:01:43Z

Pushed you patch to GitHub.

bozhiyou · 2020-07-28T01:03:22Z

Thanks! Let's move to the new PR then.

Fix assertion failure from erroneous unlock in Adaptive OBIM.

b294260

insertinterestingnamehere requested a review from bozhiyou July 23, 2020 18:39

bozhiyou reviewed Jul 26, 2020

View reviewed changes

bozhiyou requested changes Jul 26, 2020

View reviewed changes

bozhiyou added 2 commits July 26, 2020 11:50

Fix lock and divided-by-zero bugs. Rename variables by the convention…

8ad77b8

… of OBIM.

Fix init value of earliest; update code structure w.r.t OBIM

c9d964b

insertinterestingnamehere commented Jul 27, 2020

View reviewed changes

bozhiyou reviewed Jul 27, 2020

View reviewed changes

libgalois/include/galois/worklists/AdaptiveObim.h Show resolved Hide resolved

bozhiyou requested changes Jul 27, 2020

View reviewed changes

insertinterestingnamehere merged commit 1eba7f1 into IntelligentSoftwareSystems:master Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix assertion failure from erroneous unlock in Adaptive OBIM. #350

Fix assertion failure from erroneous unlock in Adaptive OBIM. #350

insertinterestingnamehere commented Jul 23, 2020

bozhiyou commented Jul 26, 2020

bozhiyou Jul 26, 2020

insertinterestingnamehere Jul 27, 2020

bozhiyou Jul 26, 2020

bozhiyou Jul 26, 2020

bozhiyou left a comment

insertinterestingnamehere Jul 27, 2020

bozhiyou Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

bozhiyou Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

bozhiyou Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

bozhiyou Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

insertinterestingnamehere Jul 27, 2020

bozhiyou left a comment

insertinterestingnamehere commented Jul 27, 2020

bozhiyou commented Jul 27, 2020

insertinterestingnamehere commented Jul 27, 2020

insertinterestingnamehere commented Jul 27, 2020

insertinterestingnamehere commented Jul 27, 2020

bozhiyou commented Jul 28, 2020

bozhiyou commented Jul 28, 2020

insertinterestingnamehere commented Jul 28, 2020

insertinterestingnamehere commented Jul 28, 2020

insertinterestingnamehere commented Jul 28, 2020

bozhiyou commented Jul 28, 2020

Fix assertion failure from erroneous unlock in Adaptive OBIM. #350

Fix assertion failure from erroneous unlock in Adaptive OBIM. #350

Conversation

insertinterestingnamehere commented Jul 23, 2020

bozhiyou commented Jul 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bozhiyou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bozhiyou left a comment

Choose a reason for hiding this comment

insertinterestingnamehere commented Jul 27, 2020

bozhiyou commented Jul 27, 2020

insertinterestingnamehere commented Jul 27, 2020

insertinterestingnamehere commented Jul 27, 2020

insertinterestingnamehere commented Jul 27, 2020

bozhiyou commented Jul 28, 2020

bozhiyou commented Jul 28, 2020

insertinterestingnamehere commented Jul 28, 2020

insertinterestingnamehere commented Jul 28, 2020

insertinterestingnamehere commented Jul 28, 2020

bozhiyou commented Jul 28, 2020