Skip to content

Conversation

@robsdedude
Copy link
Member

@robsdedude robsdedude commented May 17, 2022

The port number is (at least) queried for each debug log line. No matter if logging is enabled or not. Wrapping socket.getsockname() into a check whether debug logging is enabled or not, to only query it when necessary, made the driver slower for environments with a fast response (read: my local machine). Therefore, caching the port number is the only viable compromise. The downside is that this assumes that the port number never changes. As the driver is right now, it hands over all control over the socket to the Bolt class, so this assumption is safe. But it introduces complexity in the code base by adding the need to know about and adhere to this principle in future changes.

Background: a customer complained about the calls slowing down their deployment significantly. On my local machine it's actually really fast and commenting out the line in question (

log.debug("[#%04X] S: RECORD * %d", self.local_port, len(details))
+ in the other bolt versions) and timing a workload that hits that line frequently, didn't alter the driver's throughput in a measurable way. Timing this call in a Windows VM on my Linux machine showed it was 4 times slower. I didn't go as far as timing the whole driver in the VM.

@robsdedude robsdedude force-pushed the cache-port-number branch from 7edaed3 to 5e517db Compare May 17, 2022 07:58
@AndyHeap-NeoTech
Copy link
Contributor

The change is simple enough and looks ok to me. I'm wondering what the error would be if the port did happen to change without the cache knowing.

@robsdedude
Copy link
Member Author

The change is simple enough and looks ok to me. I'm wondering what the error would be if the port did happen to change without the cache knowing.

Currently, the port number is only used for logging. So the worst thing that could happen are wrong and maybe misleading log messages or debug sessions.

@robsdedude robsdedude marked this pull request as ready for review June 1, 2022 09:39
@robsdedude robsdedude merged commit d3a1f46 into neo4j:5.0 Jun 1, 2022
robsdedude added a commit that referenced this pull request Jun 1, 2022
@robsdedude robsdedude deleted the cache-port-number branch June 1, 2022 14: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.

2 participants