Replace scanner-based smap + annotations parsing with hand-crafted parsers #7923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
Comment on lines
197
to
200
| while (chars[i] != '-') { | ||
| buffer.append(chars[i]); | ||
| i++; | ||
| } |
There was a problem hiding this comment.
Nit - I wonder how this compares to indexOf() + substring()
But no need to change this for now - probably the perf diff won't be dramatic
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
…/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java Co-authored-by: Jaroslav Bachorik <[email protected]>
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
reviewed
Nov 8, 2024
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
jbachorik
requested changes
Nov 8, 2024
BenchmarksStartupParameters
See matching parameters
SummaryFound 12 performance improvements and 16 performance regressions! Performance is the same for 23 metrics, 12 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.035 s) : 0, 1034963
Total [baseline] (10.462 s) : 0, 10462175
Agent [candidate] (1.093 s) : 0, 1093305
Total [candidate] (10.455 s) : 0, 10454972
section appsec
Agent [baseline] (1.182 s) : 0, 1182360
Total [baseline] (10.764 s) : 0, 10764101
Agent [candidate] (1.225 s) : 0, 1224742
Total [candidate] (10.761 s) : 0, 10761430
section iast
Agent [baseline] (1.182 s) : 0, 1181698
Total [baseline] (11.055 s) : 0, 11054547
Agent [candidate] (1.223 s) : 0, 1222506
Total [candidate] (10.97 s) : 0, 10969706
section profiling
Agent [baseline] (1.26 s) : 0, 1260394
Total [baseline] (10.861 s) : 0, 10860961
Agent [candidate] (1.318 s) : 0, 1317766
Total [candidate] (10.839 s) : 0, 10839264
gantt
title petclinic - break down per module: candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (713.839 ms) : 0, 713839
BytebuddyAgent [candidate] (695.886 ms) : 0, 695886
GlobalTracer [baseline] (239.461 ms) : 0, 239461
GlobalTracer [candidate] (317.392 ms) : 0, 317392
AppSec [baseline] (55.101 ms) : 0, 55101
AppSec [candidate] (54.901 ms) : 0, 54901
Remote Config [baseline] (712.438 µs) : 0, 712
Remote Config [candidate] (682.873 µs) : 0, 683
Telemetry [baseline] (10.678 ms) : 0, 10678
Telemetry [candidate] (10.686 ms) : 0, 10686
section appsec
BytebuddyAgent [baseline] (733.795 ms) : 0, 733795
BytebuddyAgent [candidate] (711.293 ms) : 0, 711293
GlobalTracer [baseline] (237.651 ms) : 0, 237651
GlobalTracer [candidate] (313.572 ms) : 0, 313572
AppSec [baseline] (175.911 ms) : 0, 175911
AppSec [candidate] (167.507 ms) : 0, 167507
IAST [baseline] (21.686 ms) : 0, 21686
IAST [candidate] (19.338 ms) : 0, 19338
Remote Config [baseline] (652.388 µs) : 0, 652
Remote Config [candidate] (655.626 µs) : 0, 656
Telemetry [baseline] (8.318 ms) : 0, 8318
Telemetry [candidate] (8.273 ms) : 0, 8273
section iast
BytebuddyAgent [baseline] (842.802 ms) : 0, 842802
BytebuddyAgent [candidate] (815.079 ms) : 0, 815079
GlobalTracer [baseline] (233.291 ms) : 0, 233291
GlobalTracer [candidate] (306.38 ms) : 0, 306380
AppSec [baseline] (55.323 ms) : 0, 55323
AppSec [candidate] (57.152 ms) : 0, 57152
IAST [baseline] (25.286 ms) : 0, 25286
IAST [candidate] (21.91 ms) : 0, 21910
Remote Config [baseline] (648.064 µs) : 0, 648
Remote Config [candidate] (632.822 µs) : 0, 633
Telemetry [baseline] (8.925 ms) : 0, 8925
Telemetry [candidate] (7.568 ms) : 0, 7568
section profiling
BytebuddyAgent [baseline] (706.593 ms) : 0, 706593
BytebuddyAgent [candidate] (689.955 ms) : 0, 689955
GlobalTracer [baseline] (351.505 ms) : 0, 351505
GlobalTracer [candidate] (432.758 ms) : 0, 432758
AppSec [baseline] (55.236 ms) : 0, 55236
AppSec [candidate] (53.541 ms) : 0, 53541
Remote Config [baseline] (706.518 µs) : 0, 707
Remote Config [candidate] (662.814 µs) : 0, 663
Telemetry [baseline] (8.933 ms) : 0, 8933
Telemetry [candidate] (7.684 ms) : 0, 7684
ProfilingAgent [baseline] (95.073 ms) : 0, 95073
ProfilingAgent [candidate] (93.946 ms) : 0, 93946
Profiling [baseline] (95.097 ms) : 0, 95097
Profiling [candidate] (93.971 ms) : 0, 93971
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.039 s) : 0, 1039301
Total [baseline] (8.634 s) : 0, 8634447
Agent [candidate] (1.098 s) : 0, 1098211
Total [candidate] (8.667 s) : 0, 8667465
section iast
Agent [baseline] (1.179 s) : 0, 1179120
Total [baseline] (9.221 s) : 0, 9221205
Agent [candidate] (1.223 s) : 0, 1222707
Total [candidate] (9.25 s) : 0, 9250034
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.168 s) : 0, 1167582
Total [baseline] (9.171 s) : 0, 9170708
Agent [candidate] (1.221 s) : 0, 1220843
Total [candidate] (9.176 s) : 0, 9175992
section iast_TELEMETRY_OFF
Agent [baseline] (1.162 s) : 0, 1162424
Total [baseline] (9.202 s) : 0, 9201505
Agent [candidate] (1.22 s) : 0, 1219712
Total [candidate] (9.203 s) : 0, 9203055
gantt
title insecure-bank - break down per module: candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (715.265 ms) : 0, 715265
BytebuddyAgent [candidate] (699.557 ms) : 0, 699557
GlobalTracer [baseline] (241.033 ms) : 0, 241033
GlobalTracer [candidate] (318.287 ms) : 0, 318287
AppSec [baseline] (55.544 ms) : 0, 55544
AppSec [candidate] (55.179 ms) : 0, 55179
Remote Config [baseline] (713.537 µs) : 0, 714
Remote Config [candidate] (697.109 µs) : 0, 697
Telemetry [baseline] (11.565 ms) : 0, 11565
Telemetry [candidate] (10.655 ms) : 0, 10655
section iast
BytebuddyAgent [baseline] (841.632 ms) : 0, 841632
BytebuddyAgent [candidate] (814.463 ms) : 0, 814463
GlobalTracer [baseline] (232.063 ms) : 0, 232063
GlobalTracer [candidate] (306.885 ms) : 0, 306885
AppSec [baseline] (54.512 ms) : 0, 54512
AppSec [candidate] (58.346 ms) : 0, 58346
IAST [baseline] (26.076 ms) : 0, 26076
IAST [candidate] (20.964 ms) : 0, 20964
Remote Config [baseline] (633.683 µs) : 0, 634
Remote Config [candidate] (632.475 µs) : 0, 632
Telemetry [baseline] (8.911 ms) : 0, 8911
Telemetry [candidate] (7.617 ms) : 0, 7617
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (832.575 ms) : 0, 832575
BytebuddyAgent [candidate] (813.856 ms) : 0, 813856
GlobalTracer [baseline] (230.939 ms) : 0, 230939
GlobalTracer [candidate] (305.541 ms) : 0, 305541
AppSec [baseline] (53.174 ms) : 0, 53174
AppSec [candidate] (56.769 ms) : 0, 56769
IAST [baseline] (26.428 ms) : 0, 26428
IAST [candidate] (22.728 ms) : 0, 22728
Remote Config [baseline] (620.112 µs) : 0, 620
Remote Config [candidate] (627.241 µs) : 0, 627
Telemetry [baseline] (8.604 ms) : 0, 8604
Telemetry [candidate] (7.514 ms) : 0, 7514
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (829.288 ms) : 0, 829288
BytebuddyAgent [candidate] (812.63 ms) : 0, 812630
GlobalTracer [baseline] (230.405 ms) : 0, 230405
GlobalTracer [candidate] (306.163 ms) : 0, 306163
AppSec [baseline] (53.378 ms) : 0, 53378
AppSec [candidate] (57.46 ms) : 0, 57460
IAST [baseline] (24.96 ms) : 0, 24960
IAST [candidate] (21.483 ms) : 0, 21483
Remote Config [baseline] (602.951 µs) : 0, 603
Remote Config [candidate] (631.419 µs) : 0, 631
Telemetry [baseline] (8.549 ms) : 0, 8549
Telemetry [candidate] (7.537 ms) : 0, 7537
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section baseline
no_agent (1.359 ms) : 1340, 1378
. : milestone, 1359,
appsec (1.748 ms) : 1724, 1771
. : milestone, 1748,
appsec_no_iast (1.759 ms) : 1735, 1782
. : milestone, 1759,
iast (1.519 ms) : 1496, 1543
. : milestone, 1519,
profiling (1.495 ms) : 1471, 1519
. : milestone, 1495,
tracing (1.485 ms) : 1461, 1509
. : milestone, 1485,
section candidate
no_agent (1.353 ms) : 1333, 1372
. : milestone, 1353,
appsec (1.746 ms) : 1722, 1770
. : milestone, 1746,
appsec_no_iast (1.76 ms) : 1736, 1785
. : milestone, 1760,
iast (1.505 ms) : 1483, 1528
. : milestone, 1505,
profiling (1.495 ms) : 1471, 1518
. : milestone, 1495,
tracing (1.499 ms) : 1474, 1523
. : milestone, 1499,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section baseline
no_agent (375.389 µs) : 353, 397
. : milestone, 375,
iast (516.495 µs) : 493, 540
. : milestone, 516,
iast_FULL (735.693 µs) : 714, 758
. : milestone, 736,
iast_GLOBAL (553.606 µs) : 532, 575
. : milestone, 554,
iast_HARDCODED_SECRET_DISABLED (511.885 µs) : 489, 535
. : milestone, 512,
iast_INACTIVE (464.2 µs) : 442, 486
. : milestone, 464,
iast_TELEMETRY_OFF (491.902 µs) : 469, 515
. : milestone, 492,
tracing (459.536 µs) : 438, 481
. : milestone, 460,
section candidate
no_agent (378.697 µs) : 359, 398
. : milestone, 379,
iast (492.76 µs) : 471, 514
. : milestone, 493,
iast_FULL (652.824 µs) : 631, 674
. : milestone, 653,
iast_GLOBAL (521.981 µs) : 499, 544
. : milestone, 522,
iast_HARDCODED_SECRET_DISABLED (494.857 µs) : 473, 516
. : milestone, 495,
iast_INACTIVE (453.027 µs) : 432, 474
. : milestone, 453,
iast_TELEMETRY_OFF (490.745 µs) : 468, 513
. : milestone, 491,
tracing (452.768 µs) : 432, 474
. : milestone, 453,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section baseline
no_agent (15.259 s) : 15259000, 15259000
. : milestone, 15259000,
appsec (14.901 s) : 14901000, 14901000
. : milestone, 14901000,
iast (18.861 s) : 18861000, 18861000
. : milestone, 18861000,
iast_GLOBAL (18.033 s) : 18033000, 18033000
. : milestone, 18033000,
profiling (15.066 s) : 15066000, 15066000
. : milestone, 15066000,
tracing (14.811 s) : 14811000, 14811000
. : milestone, 14811000,
section candidate
no_agent (15.526 s) : 15526000, 15526000
. : milestone, 15526000,
appsec (15.036 s) : 15036000, 15036000
. : milestone, 15036000,
iast (18.658 s) : 18658000, 18658000
. : milestone, 18658000,
iast_GLOBAL (17.854 s) : 17854000, 17854000
. : milestone, 17854000,
profiling (14.989 s) : 14989000, 14989000
. : milestone, 14989000,
tracing (14.946 s) : 14946000, 14946000
. : milestone, 14946000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.44.0-SNAPSHOT~3b9b4d967b, baseline=1.47.0-SNAPSHOT~8d0bb34e88
dateFormat X
axisFormat %s
section baseline
no_agent (1.472 ms) : 1460, 1483
. : milestone, 1472,
appsec (2.362 ms) : 2319, 2406
. : milestone, 2362,
iast (2.101 ms) : 2047, 2156
. : milestone, 2101,
iast_GLOBAL (2.152 ms) : 2097, 2207
. : milestone, 2152,
profiling (1.982 ms) : 1937, 2026
. : milestone, 1982,
tracing (1.95 ms) : 1908, 1992
. : milestone, 1950,
section candidate
no_agent (1.47 ms) : 1458, 1481
. : milestone, 1470,
appsec (2.345 ms) : 2303, 2387
. : milestone, 2345,
iast (2.086 ms) : 2033, 2138
. : milestone, 2086,
iast_GLOBAL (2.129 ms) : 2076, 2183
. : milestone, 2129,
profiling (1.959 ms) : 1917, 2002
. : milestone, 1959,
tracing (1.932 ms) : 1891, 1973
. : milestone, 1932,
|
|
Any progress on this? |
… into mattalp/handcrafted-smap-parser
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
…/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>
jbachorik
requested changes
Nov 20, 2024
| System.out.println("ATTR: " + attributeName); | ||
| switch (attributeName) { | ||
| case "Size": | ||
| size = Long.decode(buffer.toString()) * 1024; |
There was a problem hiding this comment.
I see that this is still using string->stringBuffer->number conversion.
I would propose extracting the logic from parseLine method for converting the stream of characters into a number.
Something like:
long readLong(String line, int from, char delimiter, int base) throws IndexOutOfBoundsException {
long number = 0;
for (int i = from; i < line.length() && (char c = line.charAt(i)) != delimiter; i++) {
number *= base;
number += Character.digit(char, base);
}
return number;
}
With this you should also unify the approach for reading the char at i-th position. Currently, one method is using charAt(i) while the other first converts the line into char array and then accesses that array elements.
I have no strong preference for one or the other - it would just be nice to keep to the same convention :)
There was a problem hiding this comment.
I kept this section as-is because I expect the header parsing to remain stable while the KV pairs may change in format, hence the approach of loading the value into the buffer & then parsing it wholesale (with different rules depending on the attribute). Of course, this could be moved into the switch itself. Additionally, decode and parseLong won't come with the overhead of the original Scanner-based implementation.
As for the digit parsing, I agree that it could be extracted with 2 caveats:
- We would have to also return the offset into the line after reading in order to continue the parsing, which means a new (data) class to return this tuple.
- Building off of 1, I prefer to keep the indirection to a minimum here to explicitly spell out the parsing for this bespoke format (or to use easily-understood stdlib features such as
Scanner, which isn't viable for low-memory JVMs, leading to theBoxed.parseBoxed/decodeusage seen here)
We could also swap out the String for ByteBuffer as you've mentioned earlier, and that should get us position tracking "for free". WDYT?
There was a problem hiding this comment.
Let's try with ByteBuffer
There was a problem hiding this comment.
Sounds good, this will minimize whatever minimal GC churn / extra heap allocations that come from this.
...-openjdk/src/main/java/com/datadog/profiling/controller/openjdk/events/SmapEntryFactory.java
Outdated
Show resolved
Hide resolved
MattAlp
force-pushed
the
mattalp/handcrafted-smap-parser
branch
from
11c7ea0 to
d576a02
Compare
| @@ -350,6 +440,7 @@ static List<? extends Event> collectEvents() { | |||
| } catch (FileNotFoundException e) { | |||
| return List.of(new SmapParseErrorEvent(ErrorReason.SMAP_FILE_NOT_FOUND)); | |||
| } catch (Exception e) { | |||
| e.printStackTrace(); | |||
There was a problem hiding this comment.
🟠 Code Quality Violation
Avoid printStackTrace(); use a logger call instead. (...read more)
Use a logging framework instead of printStackTrace() when handling exceptions. printStackTrace() can be useful during development for quick debugging, but it is not suitable for production code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Does This Do
Replaces the scanner-and-matcher-based NMT map annotation +
/proc/smapsparsing with handcrafted parsers instead.Motivation
The usage of
Pattern,Matcher, andScannerby the smap JFR events allocates and reserves a certain amount of baseline memory that has been flagged by customers. This replaces that to use a minimal amount of regex and eliminate the heaviest allocations.Additional Notes
See PROF-10699 for an example of 'significant' memory usage.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: PROF-10699