Minor modifications in code for readability

[vedang]

Changes:

• Refactor a small section of the socket creation code to remove reliance on enc/doto-cond
• Move constants out of conn-pool to avoid re-creating objects.

Hi Peter,

I was recently looking at the Carmine connection code to understand how the underlying object pool is used by Carmine, and how Carmine manages it's connections. I made the changes that I've attached to this pull request along the way.

The one edge-case that my code change does not handle is if the user explicitly sets conn-timeout to nil in the spec. However, I couldn't tell if the original code meant to deal with this case at all.

[vedang]

I also have an associated question:

The reason I was going through this code was that I was unsure about whether the pool was working properly in my particular use-case or not. In order to see this, I added a few logging statements around the connection pooling code. I think that the logging would be generally useful to anyone curious. Should I submit the logging changes as a patch? I'm using timbre to log stuff, so another question would be - what is the performance impact if carmine always had to check whether the application had activated timbre appenders or if the timbre logging level was met?

[ptaoussanis]

Hi Vedang, thanks.

Could I just double check the motivation for moving the constants out here? Do you want to access them externally, or is this just for performance?

A change for performance isn't really going to do much; the time to create a constant object, and the time to access a var are similar. In any case, the enclosing function is cached.

I think that the logging would be generally useful to anyone curious. Should I submit the logging changes as a patch?

That's not necessary, but thank you for asking. Not sure how common it is that folks will want to log the connection pool? And if they did, they'd probably want to control the levels and so on - meaning it'd probably be simpler to just quickly write a custom ConnectionPool wrapper implementation. Does that make sense?

I'm using timbre to log stuff, so another question would be - what is the performance impact if carmine always had to check whether the application had activated timbre appenders or if the timbre logging level was met?

Depends on where you're putting it exactly (how often it runs, and how performance sensitive the area is) - but can't think of anywhere that should be a problem. You can disable your appenders/level and run (encore/qb 1e5 <your noop timbre logging call>) to benchmark the impact in your environment.

Cheers.

[vedang]

Hey Peter,

I don't want to access them externally, I've maintained the :private meta on them. It's purely meant for performance.

I realize that the gains are miniscule, but it also makes the conn-pool function easier to read IMHO, which is why I included it in the patch.

[ptaoussanis]

Gotcha, thanks for clarifying.

I do prefer the current version, but appreciate you checking (and the PR!).

Having the defaults where they are makes it immediately clear where they're used, and that they're used only there.

Re: performance-

(do
  (def my-var {:foo :bar})
  (def fn1 (fn [] my-var))
  (def fn2 (fn [] {:foo :bar})))

(encore/qb 1e6 (fn1) (fn2)) ; [39.55 39.54]

The cost of a var access and a const object creation are similar. In any event, these numbers will be noise compared to the pool creation (which is why the fn's cached).

Cheers :-)

[vedang]

Hi Peter,

The var access bit was the tag-on part of the pull request, I was interested in making the socket connection code more readable by directly using doto instead of encore's doto-cond.

I think you may have missed that bit.

[ptaoussanis]

Hi Vedang,

Thanks for clarifying, yes I did get that. To be honest I'm okay with how the code is currently and have other priorities that I'd prefer to focus on.

Appreciate the PR though, thank you :-)

Cheers!