[PERF] Don't use hmac encryption for loopback interfaces #6241
Conversation
Signed-off-by: Vadim Gimpelson <[email protected]>
WalkthroughA new utility function was introduced to detect loopback or local IPC addresses, including various protocols and address patterns. The constructor logic for Changes
Estimated code review effort2 (~12 minutes) Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tensorrt_llm/executor/ipc.py (3)
20-20: Fix grammatical error in function name.The function name
is_address_is_loopbackcontains a redundant "is". Consider renaming tois_address_loopbackfor better readability.-def is_address_is_loopback(address: str) -> bool: +def is_address_loopback(address: str) -> bool:Don't forget to update the function call on line 69 as well.
21-36: Consider edge case handling and validation for robustness.The current implementation correctly identifies the main loopback patterns for the performance optimization goal. However, consider these potential improvements for robustness:
- The IPv4 loopback check
tcp://127.*might match malformed addresses liketcp://127.abc- Missing validation for malformed address formats
Here's a more robust implementation if you want to handle edge cases:
def is_address_loopback(address: str) -> bool: """Check if the address represents a local/loopback connection.""" # IPC and inproc are always local if address.startswith(("ipc://", "inproc://")): return True # Check TCP loopback addresses if address.startswith("tcp://"): + # Extract the host part + try: + host_part = address[6:].split(':')[0] # Remove tcp:// and port + # IPv4 loopback patterns + if host_part.startswith("127.") or host_part == "localhost": + return True + # IPv6 loopback + if host_part == "[::1]": + return True + except (IndexError, ValueError): + # Malformed address, assume not loopback for safety + return False - # IPv4 loopback patterns - if (address.startswith("tcp://127.") - or address.startswith("tcp://localhost")): - return True - # IPv6 loopback - if address.startswith("tcp://[::1]"): - return True return FalseHowever, the current implementation is sufficient for the intended performance optimization use case.
69-70: Update function call and consider logging the encryption override.Two considerations for this change:
- Function name: Update the function call to match the corrected name.
- Transparency: Consider logging when encryption is automatically disabled to make the behavior visible to users.
- if is_address_is_loopback(self.address_endpoint): + if is_address_loopback(self.address_endpoint): use_hmac_encryption = False + if name: # Only log if we have a name to identify the queue + logger.debug(f"Disabled HMAC encryption for loopback address in queue '{name}'")This change overrides the user's
use_hmac_encryptionsetting for loopback addresses, which is appropriate for the performance optimization goal. The security trade-off is acceptable since loopback connections are inherently local and trusted.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tensorrt_llm/executor/ipc.py(2 hunks)
🔇 Additional comments (1)
tensorrt_llm/executor/ipc.py (1)
69-70: Performance optimization approach looks good.This change effectively addresses the performance bottleneck described in the PR objectives. Disabling HMAC encryption for loopback connections is a sound optimization that should deliver the observed performance improvements (23ms to 17ms per image) without compromising security for local communications.
Signed-off-by: Vadim Gimpelson <[email protected]>
svc-trtllm-gh-bot
added
the
Community want to contribute
Description
In
class ZeroMqQueuedisable encryption for loopback interfaces.The Reason
For Qwen-2.5-3B-VL main process (that get requests, pre-process images, send request to worker) is bottleneck. Images preprocessing and sending takes more time than inference.
sendis around 35% from all timeinside send we spend around half to sign image with
hmac.I think we can avoid encryption for loopback interfaces
Performance
Tested Qwen-2.5-3B-VL model. H100. Images 512x512
Before image processing took 23ms per image.
After 17ms
Summary by CodeRabbit