Skip to content

[Framework] Improve saferTrie node's value#71

Merged
TomerShor merged 6 commits intov3io:developmentfrom
weilerN:nuc-486-improve-safe-trie-nodes-values
Jun 24, 2025
Merged

[Framework] Improve saferTrie node's value#71
TomerShor merged 6 commits intov3io:developmentfrom
weilerN:nuc-486-improve-safe-trie-nodes-values

Conversation

@weilerN
Copy link
Collaborator

@weilerN weilerN commented Jun 22, 2025

Motivation

Described in the Jira - https://iguazio.atlassian.net/browse/NUC-486

Root Cause

Memory Layout:

[]string:        24 bytes (slice header) + 32 bytes (heap allocation for 2 strings) = 56 bytes total
CanaryTarget:    32 bytes (stack allocated, no heap allocation)

Performance Improvements:

  • Creation: Faster, avoids heap allocation
  • Access: Faster, no pointer dereferencing
  • Memory: ~43% reduction in memory footprint
  • Garbage Collection: Lower GC pressure
  • CPU Cache: Improved locality and access speed

Description

This PR introduces a memory-optimized approach for storing values in the safeTrie.
Instead of using []string to represent function names, this change replaces it with a lightweight struct-based abstraction: FunctionTarget.
It includes two concrete implementations:

  • SingleFunctionTarget – holds a single function name
  • CanaryFunctionTarget – holds exactly two function names

The FunctionTarget is propagated to the upper cache layer, and the related interfaces were renamed for clarity and consistency.

Affected Areas

Since the only usage of the FunctionTarget resides inside the cache, there are no affected areas.

Testing

  • Added unit tests for SingleFunctionTarget and CanaryFunctionTarget
  • Updated all relevant cache unit tests to align with the new structure

Changes Made

  • Introduced the FunctionTarget interface with SingleFunctionTarget and CanaryFunctionTarget implementations
  • Refactored safeTrie and cache layers to support and propagate the new interface
  • Aligned interface method declarations for readability and consistency with the FunctionTarget design

Additional Notes

  • Expanding the use of the FunctionTarget abstraction outside the cache (e.g., surfacing it in the cache interface) is left open for further discussion in the code review.

@weilerN weilerN force-pushed the nuc-486-improve-safe-trie-nodes-values branch from b03e259 to 49b4f72 Compare June 22, 2025 15:32
@weilerN weilerN force-pushed the nuc-486-improve-safe-trie-nodes-values branch from c8147c0 to e168a5f Compare June 22, 2025 15:47
@weilerN weilerN marked this pull request as ready for review June 22, 2025 15:57
Copy link
Collaborator

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

I'm not convinced that the memory improvement is this critical that this change is required.

But regardless, since you anyway use a slice string in CanaryTarget, isn't it better to only have a single, static struct?
something like:

type FunctionTarget struct {
    mainFunction string
    canaryFunction string
}

Then, you don't need the interface at all.
You can even define the as string pointers and might save even more memory.

@weilerN weilerN requested a review from rokatyy June 23, 2025 07:06
Copy link
Collaborator

@rokatyy rokatyy left a comment

Choose a reason for hiding this comment

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

Very nice! I'm actually on the side of using interface, I think it provides more flexibility and allows to hide all logic under the hood. Just one comment, other than that lgtm

Comment on lines 68 to 72
if pathFunctionNames.Contains(function) {
// If the function already exists at this path, skip adding it to prevent duplicates
return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed anymore, part of Add anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@weilerN
Copy link
Collaborator Author

weilerN commented Jun 23, 2025

I'm not convinced that the memory improvement is this critical that this change is required.

But regardless, since you anyway use a slice string in CanaryTarget, isn't it better to only have a single, static struct? something like:

type FunctionTarget struct {
    mainFunction string
    canaryFunction string
}

Then, you don't need the interface at all. You can even define the as string pointers and might save even more memory.

Thanks for the feedback, @TomerShor
While the memory gain alone may not justify the change, this PR brings two additional benefits:

  1. Maintainability and Flexibility – The interface allows each function type (single or canary) to define its own logic. This avoids sprinkling conditionals throughout the code and makes it easier to read and add new types or behaviors in the future.
  2. Encapsulation – Using separate structs (SingleFunctionTarget, CanaryFunctionTarget) avoids introducing implicit semantics like priority (e.g., mainFunction, canaryFunction).
    The FunctionTarget abstraction keeps the logic self-contained and aligned with the actual use cases.

I hope this explanation clarifies my approach — I believe these changes are beneficial, and I’d appreciate your consideration in proceeding with the proposed changes.

Copy link
Collaborator

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

Looks good and approved, but with an open question

Comment on lines 157 to 159
type SingleTarget struct {
functionName string
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can also be a wrapper for a string

Suggested change
type SingleTarget struct {
functionName string
}
type SingleTarget string

If we're talking about optimization, what is the difference between the two?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TomerShor TomerShor merged commit 03ac135 into v3io:development Jun 24, 2025
3 checks passed
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