From 130f7273179caa0ea015dc204fa7836f1e870148 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Sun, 13 Sep 2026 17:48:44 +0100 Subject: [PATCH] perf: validate actor path elements without String.charAt Motivation: ActorPath.findInvalidPathElementCharPosition runs for every named actorOf and scanned the name with String.charAt plus a pattern match with guards and a ValidSymbols.indexOf per non-alphanumeric character. String.charAt inlines to `isLatin1() ? StringLatin1.charAt : StringUTF16.charAt`, and that branch is profiled once, JVM-wide, in String.charAt's own bytecode. Any non-ASCII string handled anywhere in the process pollutes it, after which C2 compiles both coders into every charAt loop. Measured locally, the validator halves in throughput (101 -> 49 ops/us for "actor-1") once the profile is polluted. Modification: - Copy the element out once with getBytes(ISO_8859_1) (an intrinsic array copy for Latin-1 strings) and scan the byte[] with a 128-entry flag table (valid char / hex digit) instead of calling charAt per character. Characters outside Latin-1 encode as '?' and 0x80-0xFF as negative bytes, both invalid, so accepted set and reported position are unchanged. - Add directional ActorPathSpec tests for accepted names, rejected names and the reported position, including Latin-1, non-Latin-1 and surrogate-pair input. - Extend ActorPathValidationBenchmark with a `polluted` param that pre-warms String.charAt with UTF-16 strings, and keep the previous charAt-based validator there as charAtLoop* for comparison. Result: Name validation no longer depends on the String.charAt coder profile: 138-162 ops/us for "actor-1" polluted or not, versus 101 unpolluted and 49 polluted before. Also tried the same for MurmurHash.stringHash and Helpers.base64 and found no benefit (hash arithmetic and StringBuilder.append dominate), so those are unchanged. Tests: - sbt "actor-tests/testOnly org.apache.pekko.actor.ActorPathSpec org.apache.pekko.actor.LocalActorRefProviderSpec" (25 passed) - sbt "actor-tests/testOnly org.apache.pekko.routing.ConsistentHashingRouterSpec" (earlier run, passed) - sbt "actor/mimaReportBinaryIssues" (no issues) - sbt "bench-jmh/Jmh/compile" and Jmh/run of the pollution benchmark - scalafmt on changed Scala files, git diff --check References: None - performance follow-up to the hand-written actor name validator --- .../apache/pekko/actor/ActorPathSpec.scala | 54 +++++++++++++- .../org/apache/pekko/actor/ActorPath.scala | 67 +++++++++++------ .../actor/ActorPathValidationBenchmark.scala | 71 +++++++++++++++++++ 3 files changed, 168 insertions(+), 24 deletions(-) diff --git a/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala b/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala index a7573979fbf..0fbba1f7ba8 100644 --- a/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala +++ b/actor-tests/src/test/scala/org/apache/pekko/actor/ActorPathSpec.scala @@ -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 { @@ -81,6 +82,57 @@ class ActorPathSpec extends AnyWordSpec with Matchers { "must not be empty") } + "accept valid path elements" in { + 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) + } + } + "create correct toStringWithAddress" in { val local = Address("pekko", "mysys") val a = local.copy(host = Some("aaa"), port = Some(7355)) diff --git a/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala b/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala index ef43038b546..2ecc4a5cd1d 100644 --- a/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala +++ b/actor/src/main/scala/org/apache/pekko/actor/ActorPath.scala @@ -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 @@ -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. @@ -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("") diff --git a/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala b/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala index 780bbf096cd..976fd1f35ea 100644 --- a/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala +++ b/bench-jmh/src/main/scala/org/apache/pekko/actor/ActorPathValidationBenchmark.scala @@ -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 @@ -32,6 +34,21 @@ 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: + +[info] Benchmark (polluted) Mode Cnt Score Error Units +[info] ActorPathValidationBenchmark.charAtLoop7000 false thrpt 5 0.197 ± 0.028 ops/us +[info] ActorPathValidationBenchmark.charAtLoop7000 true thrpt 5 0.107 ± 0.017 ops/us +[info] ActorPathValidationBenchmark.charAtLoopActor_1 false thrpt 5 100.954 ± 14.868 ops/us +[info] ActorPathValidationBenchmark.charAtLoopActor_1 true thrpt 5 49.306 ± 8.706 ops/us +[info] ActorPathValidationBenchmark.handLoop7000 false thrpt 5 0.176 ± 0.012 ops/us +[info] ActorPathValidationBenchmark.handLoop7000 true thrpt 5 0.142 ± 0.094 ops/us +[info] ActorPathValidationBenchmark.handLoopActor_1 false thrpt 5 138.841 ± 111.054 ops/us +[info] ActorPathValidationBenchmark.handLoopActor_1 true thrpt 5 162.126 ± 281.298 ops/us */ @Fork(2) @State(JmhScope.Benchmark) @@ -41,21 +58,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 + } + }