Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[core] Micro optimizations for Node API #4353

Merged
merged 6 commits into from
Jan 27, 2023

Conversation

oowekyala
Copy link
Member

@oowekyala oowekyala commented Jan 23, 2023

Describe the PR

Mostly just make some methods of AbstractNode final so that they are easier to inline. Remove the overrides of AbstractNode::getChild, which are only seldom useful but makes the call sites in eg the implementations of NodeStream megamorphic for nothing. I found it might make a small performance difference while profiling.

Also convert AddEmptyString to a java rule because that's the slowest rule in pmd-java by very far.

I don't intend this to be groundbreaking but it should certainly be a nice to have.

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@oowekyala oowekyala changed the title [core]Micro-opts [core] Micro optimizations for Node API Jan 23, 2023
@oowekyala oowekyala added this to the 7.0.0 milestone Jan 23, 2023
@oowekyala oowekyala marked this pull request as ready for review January 23, 2023 17:13
@pmd-test
Copy link

pmd-test commented Jan 23, 2023

2 Messages
📖 Compared to pmd/7.0.x:
This changeset changes 11 violations,
introduces 1 new violations, 4 new errors and 0 new configuration errors,
removes 1207 violations, 0 errors and 0 configuration errors.
Full report
📖 Compared to master:
This changeset changes 49558 violations,
introduces 33872 new violations, 1450 new errors and 0 new configuration errors,
removes 197070 violations, 4 errors and 7 configuration errors.
Full report
Compared to pmd/7.0.x:
This changeset changes 17 violations,
introduces 40064 new violations, 5 new errors and 0 new configuration errors,
removes 95 violations, 1438 errors and 0 configuration errors.
Full report
Compared to master:
This changeset changes 58151 violations,
introduces 36780 new violations, 13 new errors and 0 new configuration errors,
removes 157022 violations, 4 errors and 7 configuration errors.
Full report

Generated by 🚫 Danger

@jsotuyod
Copy link
Member

jsotuyod commented Jan 27, 2023

I'm curious about the workload against which you got this rule to be "the slowest by very far". Don't get me wrong, it's certainly among the slowest, but when I ran PMD 7 against it's own sources, there were 8 rules even slower than that one:

AddEmptyString                                          1.1624           1.1624    3,530       3,530
AvoidArrayLoops                                         1.1930           1.1930    3,530       3,530
UseStringBufferLength                                   1.2310           1.2310    3,530       3,530
MissingStaticMethodInNonInstantiatableClass             1.3730           1.3730    3,530       3,530
AvoidLiteralsInIfCondition                              1.4089           1.4089    3,530       3,530
DoNotTerminateVM                                        1.4655           1.4655    3,530       3,530
ControlStatementBraces                                  1.6419           1.6419    3,530       3,530
DoNotUseThreads                                         1.6676           1.6676  234,458     234,458
UnnecessaryImport                                       1.8470           1.8470    3,530       3,530

ControlStatementBraces and AvoidLiteralsInIfCondition actually improve a lot with #4362, with ControlStatementBraces being the main reason why I wrote that PR, as I was shocked to see that rule ~7X slower than in PMD 6, when PMD 7 is ~2X faster overall, and most rules significantly faster than in PMD 6.

@jsotuyod jsotuyod merged commit 85b4162 into pmd:pmd/7.0.x Jan 27, 2023
@oowekyala oowekyala deleted the pmd7.micro-opts branch January 28, 2023 04:22
@oowekyala
Copy link
Member Author

I've got this on openjdk 11 sources of java.base

UnnecessaryImport                                       1,3385           1,3385    2 882       2 882
AvoidLiteralsInIfCondition                              1,4913           1,4913    2 882       2 882
UseStringBufferLength                                   1,6275           1,6275    2 882       2 882
CloseResource                                           1,6513           1,6513    2 882       2 882
AvoidArrayLoops                                         1,6710           1,6710    2 882       2 882
MissingStaticMethodInNonInstantiatableClass             1,8117           1,8117    2 882       2 882
ControlStatementBraces                                  1,8251           1,8251    2 882       2 882
DoNotTerminateVM                                        1,9195           1,9195    2 882       2 882
UnusedAssignment                                        1,9914           1,9914    2 882       2 882
DoNotUseThreads                                         2,3252           2,3252  358 125     358 125
AddEmptyString                                          2,7917           2,7917    2 882       2 882

Interestingly I ran on pmd sources like you and there AddEmptyString was rank 13 so not sticking out. Maybe the jdk sources have some pathological files...

@jsotuyod
Copy link
Member

Thanks! Very interesting to see how PMD performs differently under different workloads.

@oowekyala oowekyala mentioned this pull request Mar 11, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants