Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

DiplomaticNetworkNodeMapping fails with more than 9 cores #54

Open
mayyxeng opened this issue Sep 1, 2023 · 2 comments
Open

DiplomaticNetworkNodeMapping fails with more than 9 cores #54

mayyxeng opened this issue Sep 1, 2023 · 2 comments

Comments

@mayyxeng
Copy link

mayyxeng commented Sep 1, 2023

Method getNode in DiplomaticNetworkNodeMapping cannot handle more than 9 cores. The issue is with how string matching is performed, consider:

getNode("Core 10 DCache[0],Core 10 DCache MMIO[0],Core 10 ICache[0]", ListMap("Core 10" -> 1, "Core 1" -> 0))

Here we will have matches = List(true, true), because "Core 10" contains both "Core 1" and "Core 10". I would say the desired results should have been matches = List(true, false).

A quick fix is:

 def getNode(l: String, nodeMapping: ListMap[String, Int]): Option[Int] = {
    val keys = nodeMapping.keys.toSeq
    val patterns = keys.map { k =>
      val regexSpecial = Seq(
        "+","*", "?", "^", "$", "(", ")", "[", "]", "{", "}","|"
      )
      val escaped = regexSpecial.foldLeft(k) { case (kk, ch) =>
        kk.replace(ch, "\\" + ch)
      }
      raw".*${escaped}[A-Z,a-z,\,,\[,\],\s,\|]+.*".r
    }
    val matches = patterns.map { p => p.findFirstIn(l) }
    if (matches.filter(_.nonEmpty).size == 1) {
      val index = matches.indexWhere(_.nonEmpty)
      Some(nodeMapping.values.toSeq(index))
    } else {
      None
    }
  }
@jerryz123
Copy link
Collaborator

Thanks for reporting and posting a fix.

I don't remember precisely, but does "Core 1 " (note the space) resolve this too? It's quite the hack, but I seem to recall that's how I got around this issue.

Obviously the more fundamental problem is using this string matching system in the first place... I couldn't find a better way to do this without major API additions elsewhere.

@mayyxeng
Copy link
Author

mayyxeng commented Sep 1, 2023

Thanks for the quick response.

Yes "Core 1 " (with white-space) does solve the issue. And that is a much simpler and better hack than regular expression.

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

No branches or pull requests

2 participants