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

Disable ivy log4j caller location calculation #132

Merged
merged 4 commits into from Nov 9, 2017

Conversation

@leonardehrenfried
Copy link
Contributor

commented Nov 8, 2017

This is a reworking of #131 which is getting closer to the root cause of issue: If you look a little closer at sbt/sbt#3711 you can see that most time is spent calculating the caller location for the async logger.

AFAICS the caller location is never displayed anywhere so it can be discarded.

https://issues.apache.org/jira/browse/LOG4J2-153 however indicates this information should be constructed lazily, so I'm still a little puzzled why this takes so long.

@leonardehrenfried

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2017

I also added a unit test which allows you to easily test the performance of the logging method.

On my machine logging 100k messages takes 5331ms with calculating location and 204ms without, so the gains are substantial.

Are there any general performance benchmarks akin to https://developer.lightbend.com/blog/2017-06-12-faster-scala-compiler/ in place?

@pshirshov

This comment has been minimized.

Copy link

commented Nov 8, 2017

I still would say that it's a good idea to keep your previous patch and moreover move it one level up, into util.Logger. You don't know what else may change in future and your threshold checking is completely sane and logical. Yes, now you are fixing root cause but I would say it's a good idea to protect from possible changes/mistakes in future.

Also thank you for this.

@eed3si9n
Copy link
Member

left a comment

Thanks for this change. LGTM.

@eed3si9n eed3si9n requested a review from dwijnand Nov 8, 2017

@leonardehrenfried

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2017

@pshirshov Eugene has explained here #131 (review) why my previous patch doesn't work.

@dwijnand dwijnand changed the base branch from 1.x to 1.0.x Nov 8, 2017

@dwijnand dwijnand changed the base branch from 1.0.x to 1.x Nov 8, 2017

@dwijnand

This comment has been minimized.

Copy link
Member

commented Nov 8, 2017

I think we should get this into sbt 1.0.4. @leonardehrenfried, thanks for the change - could you rebase this onto 1.0.x please?

@leonardehrenfried leonardehrenfried force-pushed the leonardehrenfried:disable-location branch from ee7b77a to 7ffa2c6 Nov 8, 2017

@leonardehrenfried leonardehrenfried force-pushed the leonardehrenfried:disable-location branch from 7ffa2c6 to c1966b6 Nov 8, 2017

@leonardehrenfried leonardehrenfried changed the base branch from 1.x to 1.0.x Nov 8, 2017

@leonardehrenfried

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2017

I've rebased onto 1.0.x. and changed the base branch.

@dwijnand Let me know if I got it wrong because I saw that you also changed the base branch.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

No, that's great @leonardehrenfried - I changed the base and realised it would've brought in unrelated 1.x changed into 1.0.x, so I switched back.

@dwijnand dwijnand merged commit bf4f4a5 into sbt:1.0.x Nov 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leonardehrenfried leonardehrenfried deleted the leonardehrenfried:disable-location branch Nov 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.