Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ package org.apache.pekko.actor
import java.net.MalformedURLException

import org.scalatest.matchers.should.Matchers
import org.scalatest.prop.TableDrivenPropertyChecks
import org.scalatest.wordspec.AnyWordSpec

class ActorPathSpec extends AnyWordSpec with Matchers {
class ActorPathSpec extends AnyWordSpec with Matchers with TableDrivenPropertyChecks {

"An ActorPath" must {

Expand Down Expand Up @@ -81,6 +82,83 @@ class ActorPathSpec extends AnyWordSpec with Matchers {
"must not be empty")
}

"accept valid path elements" in {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider adding an exhaustive sweep over all 65,536 chars asserting isValidPathElement matches an independently computed predicate; it runs in milliseconds and would permanently lock in equivalence with the previous charAt implementation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added an exhaustive sweep over all 65,536 chars in 5199810 (branch rebased on main too). It checks isValidPathElement against an independently defined valid-char/hex-digit predicate for each char at position 0, after another char, and in both hex positions of a %XX escape. Runs in under a second.

val valid = Table(
"name",
"a",
"actor-1",
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789",
"-_.*$+:@&=,!~';",
"a$" + "b",
"%20",
"%ff%FF%0a",
"a%20b",
"x" * 10000)
forAll(valid) { name =>
ActorPath.isValidPathElement(name) should ===(true)
ActorPath.validatePathElement(name)
}
}

"reject invalid path elements and report the position" in {
def positionOf(name: String): Int =
intercept[InvalidActorNameException](ActorPath.validatePathElement(name)).getMessage match {
case msg if msg.contains("at position: ") =>
msg.split("at position: ")(1).takeWhile(_.isDigit).toInt
case msg => fail(s"unexpected message [$msg]")
}

val invalid = Table(
("name", "position"),
("$" + "a", 0), // `$` is reserved for system names at the start
("a b", 1),
("a/b", 1),
("a#b", 1),
("a?b", 1),
("a%", 1), // `%` is only valid as `%XX`
("a%2", 1),
("a%2g", 1),
("a%g2", 1),
("caf\u00e9", 3), // Latin-1 but not ASCII
("na\u00efve-actor", 2),
("\u4e2d\u6587", 0), // outside Latin-1
("actor-\u4e2d", 6),
("actor-\ud83d\ude00", 6), // surrogate pair
("a\u0000b", 1),
("a\u007fb", 1),
("x" * 100 + "\u00e9", 100))
forAll(invalid) { (name, position) =>
ActorPath.isValidPathElement(name) should ===(false)
positionOf(name) should ===(position)
}
}

"classify every char like the reference predicate" in {
// Independent, exhaustive definition of the accepted alphabet, so the table-driven validator
// is locked to it for every one of the 65,536 chars (including 0x80-0xFF and surrogates).
def isValidChar(c: Char): Boolean =
(c >= 'a' && c <= 'z') ||
(c >= 'A' && c <= 'Z') ||
(c >= '0' && c <= '9') ||
"-_.*$+:@&=,!~';".contains(c)
def isHexChar(c: Char): Boolean =
(c >= 'a' && c <= 'f') ||
(c >= 'A' && c <= 'F') ||
(c >= '0' && c <= '9')

var c = 0
while (c <= Char.MaxValue) {
val ch = c.toChar
withClue(f"char U+$c%04X: ") {
ActorPath.isValidPathElement(ch.toString) should ===(isValidChar(ch) && ch != '$')
ActorPath.isValidPathElement("a" + ch) should ===(isValidChar(ch))
ActorPath.isValidPathElement("%" + ch + "0") should ===(isHexChar(ch))
ActorPath.isValidPathElement("%0" + ch) should ===(isHexChar(ch))
}
c += 1
}
}

"create correct toStringWithAddress" in {
val local = Address("pekko", "mysys")
val a = local.copy(host = Some("aaa"), port = Some(7355))
Expand Down
67 changes: 44 additions & 23 deletions actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.apache.pekko.actor
import java.lang.{ StringBuilder => JStringBuilder }
import java.net.MalformedURLException
import java.nio.charset.StandardCharsets

import scala.annotation.nowarn
import scala.annotation.tailrec
Expand Down Expand Up @@ -79,6 +80,26 @@ object ActorPath {
private final val ValidPathCode = -1
private final val EmptyPathCode = -2

private final val ValidCharFlag: Byte = 1
private final val HexCharFlag: Byte = 2

/**
* Lookup table over the 7-bit ASCII range: bit 0 marks a character allowed in a path element,
* bit 1 marks a hex digit (for `%XX` escapes). Everything at or above 0x80 is invalid.
*/
private final val PathCharFlags: Array[Byte] = {
val flags = new Array[Byte](128)
def set(c: Char, flag: Byte): Unit = flags(c) = (flags(c) | flag).toByte
('a' to 'z').foreach(set(_, ValidCharFlag))
('A' to 'Z').foreach(set(_, ValidCharFlag))
('0' to '9').foreach(set(_, ValidCharFlag))
ValidSymbols.foreach(set(_, ValidCharFlag))
('a' to 'f').foreach(set(_, HexCharFlag))
('A' to 'F').foreach(set(_, HexCharFlag))
('0' to '9').foreach(set(_, HexCharFlag))
flags
}

/**
* Validates the given actor path element and throws an [[InvalidActorNameException]] if invalid.
* See [[#isValidPathElement]] for a non-throwing version.
Expand Down Expand Up @@ -127,29 +148,29 @@ object ActorPath {
private final def findInvalidPathElementCharPosition(s: String): Int =
if (s.isEmpty) EmptyPathCode
else {
def isValidChar(c: Char): Boolean =
(c >= 'a' && c <= 'z') ||
(c >= 'A' && c <= 'Z') ||
(c >= '0' && c <= '9') ||
(ValidSymbols.indexOf(c) != -1)

def isHexChar(c: Char): Boolean =
(c >= 'a' && c <= 'f') ||
(c >= 'A' && c <= 'F') ||
(c >= '0' && c <= '9')

val len = s.length
def validate(pos: Int): Int =
if (pos < len)
s.charAt(pos) match {
case c if isValidChar(c) => validate(pos + 1)
case '%' if pos + 2 < len && isHexChar(s.charAt(pos + 1)) && isHexChar(s.charAt(pos + 2)) =>
validate(pos + 3)
case _ => pos
}
else ValidPathCode

if (len > 0 && s.charAt(0) != '$') validate(0) else 0
// Copy the string out once (an intrinsic array copy for Latin-1 strings) and scan the byte
// array with a table lookup. This avoids `String.charAt` in the loop, whose Latin-1/UTF-16
// coder branch shares one JVM-wide profile and is easily polluted by unrelated non-ASCII
// strings. Characters outside Latin-1 are encoded as '?', which is invalid, and characters
// in 0x80-0xFF are negative bytes, which are also invalid, so positions are preserved.
val bytes = s.getBytes(StandardCharsets.ISO_8859_1)
val len = bytes.length
val flags = PathCharFlags

def flagsAt(pos: Int): Int = {
val b = bytes(pos)
if (b >= 0) flags(b) else 0
}

@tailrec def validate(pos: Int): Int =
if (pos >= len) ValidPathCode
else if ((flagsAt(pos) & ValidCharFlag) != 0) validate(pos + 1)
else if (bytes(pos) == '%' && pos + 2 < len &&
(flagsAt(pos + 1) & HexCharFlag) != 0 && (flagsAt(pos + 2) & HexCharFlag) != 0)
validate(pos + 3)
else pos

if (bytes(0) != '$') validate(0) else 0
}

private[pekko] final val emptyActorPath: immutable.Iterable[String] = List("")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import org.openjdk.jmh.annotations.Fork
import org.openjdk.jmh.annotations.Measurement
import org.openjdk.jmh.annotations.Mode
import org.openjdk.jmh.annotations.OutputTimeUnit
import org.openjdk.jmh.annotations.Param
import org.openjdk.jmh.annotations.Setup
import org.openjdk.jmh.annotations.State
import org.openjdk.jmh.annotations.Warmup

Expand All @@ -32,6 +34,23 @@ import org.openjdk.jmh.annotations.Warmup

[info] a.a.ActorPathValidationBenchmark.handLoopActor_1 thrpt 20 38.825 3.378 ops/us
[info] a.a.ActorPathValidationBenchmark.oldActor_1 thrpt 20 1.585 0.090 ops/us

`polluted = true` first exercises String.charAt with UTF-16 strings so its Latin-1/UTF-16 coder branch
profile (collected once, JVM-wide, in String.charAt's own bytecode) sees both coders, as it would in
any process that ever handles non-ASCII text. charAt-based scanners then get both coders compiled into
their loop; the byte-table scanner (isValidPathElement) is unaffected. For very long names the
byte-table scanner pays for the getBytes copy and is slower than an unpolluted charAt loop
(-f 3 -wi 5 -w 2s -i 10 -r 2s):

[info] Benchmark (polluted) Mode Cnt Score Error Units
[info] ActorPathValidationBenchmark.charAtLoop7000 false thrpt 30 0.152 ± 0.003 ops/us
[info] ActorPathValidationBenchmark.charAtLoop7000 true thrpt 30 0.086 ± 0.004 ops/us
[info] ActorPathValidationBenchmark.charAtLoopActor_1 false thrpt 30 108.294 ± 4.260 ops/us
[info] ActorPathValidationBenchmark.charAtLoopActor_1 true thrpt 30 41.834 ± 0.544 ops/us
[info] ActorPathValidationBenchmark.handLoop7000 false thrpt 30 0.101 ± 0.014 ops/us
[info] ActorPathValidationBenchmark.handLoop7000 true thrpt 30 0.096 ± 0.007 ops/us
[info] ActorPathValidationBenchmark.handLoopActor_1 false thrpt 30 160.988 ± 6.836 ops/us
[info] ActorPathValidationBenchmark.handLoopActor_1 true thrpt 30 161.953 ± 3.548 ops/us
*/
@Fork(2)
@State(JmhScope.Benchmark)
Expand All @@ -41,21 +60,75 @@ import org.openjdk.jmh.annotations.Warmup
@OutputTimeUnit(TimeUnit.MICROSECONDS)
class ActorPathValidationBenchmark {

@Param(Array("false", "true"))
var polluted: Boolean = false

final val a = "actor-1"
final val s = "687474703a2f2f74686566727569742e636f6d2f26683d37617165716378357926656e" * 100

final val ElementRegex = """(?:[-\w:@&=+,.!~*'_;]|%\p{XDigit}{2})(?:[-\w:@&=+,.!~*'$_;]|%\p{XDigit}{2})*""".r

@Setup
def setup(): Unit = {
if (polluted) {
val utf16 = "årsrapport-ünïcödé-中文-é"
var acc = 0
var i = 0
while (i < 2000000) {
acc += (if (charAtLoop(utf16 + i)) 1 else 0) + utf16.charAt(i % utf16.length)
i += 1
}
if (acc == 42) println("unlikely")
}
}

// @Benchmark // blows up with stack overflow, we know
def old7000: Option[List[String]] = ElementRegex.unapplySeq(s)

@Benchmark
def handLoop7000: Boolean = ActorPath.isValidPathElement(s)

@Benchmark
def charAtLoop7000: Boolean = charAtLoop(s)

@Benchmark
def oldActor_1: Option[List[String]] = ElementRegex.unapplySeq(a)

@Benchmark
def handLoopActor_1: Boolean = ActorPath.isValidPathElement(a)

@Benchmark
def charAtLoopActor_1: Boolean = charAtLoop(a)

// the String.charAt based validator that isValidPathElement used before the byte-table version
private final val ValidSymbols = """-_.*$+:@&=,!~';"""

private def charAtLoop(s: String): Boolean =
if (s.isEmpty) false
else {
def isValidChar(c: Char): Boolean =
(c >= 'a' && c <= 'z') ||
(c >= 'A' && c <= 'Z') ||
(c >= '0' && c <= '9') ||
(ValidSymbols.indexOf(c) != -1)

def isHexChar(c: Char): Boolean =
(c >= 'a' && c <= 'f') ||
(c >= 'A' && c <= 'F') ||
(c >= '0' && c <= '9')

val len = s.length
def validate(pos: Int): Int =
if (pos < len)
s.charAt(pos) match {
case c if isValidChar(c) => validate(pos + 1)
case '%' if pos + 2 < len && isHexChar(s.charAt(pos + 1)) && isHexChar(s.charAt(pos + 2)) =>
validate(pos + 3)
case _ => pos
}
else -1

(if (len > 0 && s.charAt(0) != '$') validate(0) else 0) == -1
}

}
Loading