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

Reduce unnecessary string allocation when debug is disabled in MessageSocket #761

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

mpilquist
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Merging #761 (755e041) into main (d9b5b59) will decrease coverage by 2.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
- Coverage   87.18%   85.12%   -2.06%     
==========================================
  Files         111      124      +13     
  Lines        1506     1661     +155     
  Branches      125      121       -4     
==========================================
+ Hits         1313     1414     +101     
- Misses        193      247      +54     
Impacted Files Coverage Δ
...core/shared/src/main/scala/net/MessageSocket.scala 100.00% <100.00%> (ø)
modules/core/shared/src/main/scala/Fragment.scala 100.00% <0.00%> (ø)
...dules/core/shared/src/main/scala/Transaction.scala 30.95% <0.00%> (ø)
...shared/src/main/scala-2.13+/data/ArrPlatform.scala 100.00% <0.00%> (ø)
...re/shared/src/main/scala/codec/NumericCodecs.scala 100.00% <0.00%> (ø)
...odules/core/js-jvm/src/main/scala-2/SqlState.scala 100.00% <0.00%> (ø)
...red/src/main/scala-2/syntax/StringContextOps.scala 100.00% <0.00%> (ø)
modules/core/shared/src/main/scala/Channel.scala 95.23% <0.00%> (ø)
...s/refined/src/main/scala/codec/NumericCodecs.scala 0.00% <0.00%> (ø)
...s/core/shared/src/main/scala-2/util/Twiddler.scala 100.00% <0.00%> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

): F[MessageSocket[F]] =
Queue.circularBuffer[F, Either[Any, Any]](10).map { cb =>
new AbstractMessageSocket[F] with MessageSocket[F] {

private def debug(msg: => String): F[Unit] =
Console[F].println(msg).whenA(debugEnabled)
Copy link

@ybasket ybasket Dec 16, 2022

Choose a reason for hiding this comment

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

Wouldn't that evaluate msg still as println takes a strict argument? I expected something like

Suggested change
Console[F].println(msg).whenA(debugEnabled)
Applicative[F].whenA(debugEnabled)(Console[F].println(msg))

which takes a => F[A].

Edit: Sorry, forget this, I was looking at an outdated version of the whenA syntax.

Copy link
Member

Choose a reason for hiding this comment

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

I think there is still a thunk allocation here, which we can avoid. There's also the thunk for the string, which we are stuck with unless we reach for macros.

Suggested change
Console[F].println(msg).whenA(debugEnabled)
if (debugEnabled) Console[F].println(msg) else Concurrent[F].unit

@mpilquist mpilquist merged commit 47b9810 into main Dec 16, 2022
@mpilquist mpilquist deleted the topic/guard-debug-logs branch December 16, 2022 16:12
@mpilquist mpilquist restored the topic/guard-debug-logs branch December 16, 2022 19:02
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.

3 participants