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

Apply some micro-optimizations to logging setup code #17918

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jun 15, 2021

  • Replace map iteration using entrySet with forEach (@Sanne has mentioned that this is much faster)
  • Get rid of lambdas
  • Don't allocate various datastructures unless necessary

I took this up when looking at #17914

@Sanne
Copy link
Member

Sanne commented Jun 15, 2021

Replace map iteration using entrySet with forEach (@Sanne has mentioned that this is much faster)

hum let me qualify that :) I said it's sometimes better because the forEach idiom can avoid allocating an Iterator - but iterators aren't super expensive so this would still depend on context and other details.
In this case to skip allocating an iterator it seems you have to allocate other objects to support this - including multiple BiFunction instances - so it might very well be that the entrySet approach was cheaper?

I just glanced at it though so I'm not sure - just making sure what I said isn't misunderstood :)

@Sanne
Copy link
Member

Sanne commented Jun 15, 2021

BTW a great way to make it easy for a reviewer to actually see the refactoring will be beneficial is to use methodhandles, as they won't hide capturing lambdas. Also if you can't crack the puzzle with methodhandles, it's a hint that you're needing to allocate something else for context... so I really like them

@geoand
Copy link
Contributor Author

geoand commented Jun 15, 2021

Replace map iteration using entrySet with forEach (@Sanne has mentioned that this is much faster)

hum let me qualify that :) I said it's sometimes better because the forEach idiom can avoid allocating an Iterator - but iterators aren't super expensive so this would still depend on context and other details.
In this case to skip allocating an iterator it seems you have to allocate other objects to support this - including multiple BiFunction instances - so it might very well be that the entrySet approach was cheaper?

I took a look at what entrySet() needs to allocate in order to support it's operation and it seems to be much more than the single BiConsumer that is needed here :)

* Replace map iteration using entrySet with forEach (@Sanne has mentioned that this is much faster)
* Get rid of lambdas
* Don't allocate various datastructures unless necessary
Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

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

Approved pending CI (I didn't test it!)

Thanks @geoand !

@Sanne Sanne added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 15, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 15, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building ff0064a

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Devtools Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
Gradle Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Gradle Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs
Maven Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs
Native Tests - Amazon Build ⚠️ Check → Logs Raw logs
Native Tests - Cache ⚠️ Check → Logs Raw logs
Native Tests - Data1 Set up JDK 11 ⚠️ Check → Logs Raw logs
Native Tests - Data2 Build ⚠️ Check → Logs Raw logs
Native Tests - Data3 Build ⚠️ Check → Logs Raw logs
Native Tests - Data4 Build ⚠️ Check → Logs Raw logs
Native Tests - Data5 Build ⚠️ Check → Logs Raw logs
Native Tests - Data6 Build ⚠️ Check → Logs Raw logs
Native Tests - Data7 Build ⚠️ Check → Logs Raw logs
Native Tests - HTTP ⚠️ Check → Logs Raw logs
Native Tests - Main Build ⚠️ Check → Logs Raw logs
Native Tests - Messaging1 ⚠️ Check → Logs Raw logs
Native Tests - Messaging2 ⚠️ Check → Logs Raw logs
Native Tests - Misc1 ⚠️ Check → Logs Raw logs
Native Tests - Misc2 ⚠️ Check → Logs Raw logs
Native Tests - Misc3 ⚠️ Check → Logs Raw logs
Native Tests - Misc4 ⚠️ Check → Logs Raw logs
Native Tests - Security1 ⚠️ Check → Logs Raw logs
Native Tests - Security2 ⚠️ Check → Logs Raw logs
Native Tests - Security3 ⚠️ Check → Logs Raw logs
Native Tests - Spring ⚠️ Check → Logs Raw logs
Native Tests - Windows - hibernate-validator ⚠️ Check → Logs Raw logs
Native Tests - gRPC ⚠️ Check → Logs Raw logs

@geoand
Copy link
Contributor Author

geoand commented Jun 15, 2021

Thanks @geoand !

Thanks for checking!

@geoand geoand merged commit 282766a into quarkusio:main Jun 15, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jun 15, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 15, 2021
@geoand geoand deleted the logging-microopt branch June 15, 2021 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants