Skip to content

perf: Remove preemptive deepcopy operations from exported methods#361

Draft
Copilot wants to merge 6 commits intomainfrom
copilot/fix-311
Draft

perf: Remove preemptive deepcopy operations from exported methods#361
Copilot wants to merge 6 commits intomainfrom
copilot/fix-311

Conversation

Copy link

Copilot AI commented Jun 3, 2025

This PR removes preemptive deepcopy() operations from exported methods in ExtendedDataSquare to significantly improve performance by eliminating unnecessary memory allocations.

Changes Made

Performance Optimizations

  • Removed deepcopy from exported methods: Row(), Col(), RowRoots(), ColRoots(), and Flattened() now return direct references to internal data instead of expensive copies
  • Preserved deepCopy() utility function: Enhanced with comprehensive documentation for callers who need to copy data before modification

API Documentation Updates

All affected methods now clearly document that:

  • Returned data references internal structures and should not be modified
  • Callers should use deepCopy() when modification is required
  • This shifts the performance cost only to those who actually need copies

Test Suite Updates

  • Removed obsolete tests: TestImmutableRoots and TestEDSRowColImmutable which tested the old immutable behavior
  • Added new verification: TestDirectReferences demonstrates the new behavior and proper usage patterns

Performance Impact

Benchmarking shows dramatic performance improvements:

  • Row access: ~6,000x faster (0.8 ns vs 4,841 ns)
  • RowRoots access: ~420x faster (2.8 ns vs 1,179 ns)

Migration Guide

Before:

row := eds.Row(0)
row[0][0] = newValue // This was safe because Row() returned a copy

After:

row := eds.Row(0)
// For read-only access, no changes needed - much faster now!

// For modification, explicitly copy:
rowCopy := deepCopy(eds.Row(0))
rowCopy[0][0] = newValue // Safe to modify the copy

This change maintains the same API surface while dramatically improving performance for the common read-only use case. All existing functionality remains intact.

Fixes #311.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits June 3, 2025 09:00
Co-authored-by: musalbas <5038597+musalbas@users.noreply.github.com>
Co-authored-by: musalbas <5038597+musalbas@users.noreply.github.com>
Copilot AI changed the title [WIP] perf: Consider removing deepcopy perf: Remove preemptive deepcopy operations from exported methods Jun 3, 2025
Copilot AI requested a review from musalbas June 3, 2025 09:02
Co-authored-by: adlerjohn <3290375+adlerjohn@users.noreply.github.com>
@adlerjohn
Copy link
Contributor

@copilot can you update the example in the README to use and explain the newly-exported DeepCopy for one of the returned RowRoots or ColRoots?

Update README to demonstrate proper usage of the exported DeepCopy function
with RowRoots, showing the pattern for read-only vs modification scenarios.

Co-authored-by: adlerjohn <3290375+adlerjohn@users.noreply.github.com>
Copy link
Author

Copilot AI commented Jun 3, 2025

@copilot can you update the example in the README to use and explain the newly-exported DeepCopy for one of the returned RowRoots or ColRoots?

I've updated the README example to demonstrate the proper usage of the exported DeepCopy function with RowRoots(). The example now shows both the fast read-only access pattern and the safe modification pattern using DeepCopy(). See commit 1752d10.

@adlerjohn adlerjohn marked this pull request as ready for review June 3, 2025 12:02
@adlerjohn adlerjohn requested review from Wondertan and walldiss June 3, 2025 12:02
@musalbas musalbas requested a review from Copilot June 3, 2025 12:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes performance by removing unnecessary deepcopy() calls in exported methods of ExtendedDataSquare, ensuring direct data access for read-only use while preserving a DeepCopy utility for safe modifications.

  • Removed preemptive copies in Row(), Col(), RowRoots(), ColRoots(), and Flattened() for faster access
  • Kept and documented a public DeepCopy() helper for callers needing mutable copies
  • Updated tests to demonstrate new direct-reference behavior and adjusted the README example accordingly

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
extendeddatasquare.go Dropped internal deepcopy in API methods; added doc comments and DeepCopy utility
extendeddatasquare_test.go Removed old immutability tests; added TestDirectReferences to validate direct references and copying
README.md Enhanced example to show using DeepCopy for safe modification

row2 := eds.Row(0)

// Should be the same underlying slice (direct reference)
require.True(t, &row1[0] == &row2[0], "Row() should return direct references")
Copy link

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

The test compares slice header addresses (&row1[0] vs &row2[0]) which are always different; to verify shared underlying data, compare byte pointers, e.g., &row1[0][0] == &row2[0][0].

Suggested change
require.True(t, &row1[0] == &row2[0], "Row() should return direct references")
require.True(t, &row1[0][0] == &row2[0][0], "Row() should return direct references")

Copilot uses AI. Check for mistakes.
col2 := eds.Col(0)

// Should be the same underlying slice (direct reference)
require.True(t, &col1[0] == &col2[0], "Col() should return direct references")
Copy link

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

Similar to the Row test, compare byte-level pointers (&col1[0][0] == &col2[0][0]) to ensure the underlying slice data is shared.

Suggested change
require.True(t, &col1[0] == &col2[0], "Col() should return direct references")
require.True(t, reflect.DeepEqual(col1, col2), "Col() should return direct references")

Copilot uses AI. Check for mistakes.
require.NoError(t, err)

// Should be the same underlying slice (direct reference)
require.True(t, &roots1[0] == &roots2[0], "RowRoots() should return direct references")
Copy link

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

The test currently checks slice element addresses; to confirm shared data, compare the first byte in the root slices (&roots1[0][0] == &roots2[0][0]).

Suggested change
require.True(t, &roots1[0] == &roots2[0], "RowRoots() should return direct references")
require.True(t, &roots1[0][0] == &roots2[0][0], "RowRoots() should return direct references")

Copilot uses AI. Check for mistakes.
require.NoError(t, err)

// Should be the same underlying slice (direct reference)
require.True(t, &roots1[0] == &roots2[0], "ColRoots() should return direct references")
Copy link

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

Adjust this assertion to compare byte pointers (&roots1[0][0] == &roots2[0][0]) so the test accurately validates direct reference sharing.

Suggested change
require.True(t, &roots1[0] == &roots2[0], "ColRoots() should return direct references")
require.True(t, reflect.DeepEqual(roots1[0], roots2[0]), "ColRoots() should return direct references")

Copilot uses AI. Check for mistakes.
@adlerjohn adlerjohn marked this pull request as draft June 5, 2025 01:10
@adlerjohn
Copy link
Contributor

Col/ColRoots/Row/RowRoots are now pass-throughs of the internal methods, so the internal methods are redundant.

Copy link
Author

Copilot AI commented Jun 5, 2025

Col/ColRoots/Row/RowRoots are now pass-throughs of the internal methods, so the internal methods are redundant.

You're absolutely right! I've simplified the code by removing the redundant internal methods where possible. The row() and col() methods have been completely removed, and the exported methods now call the underlying rowSlice() and colSlice() methods directly. For the root methods, I've inlined the caching logic directly into the exported methods for better performance. See commit coming up.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • http://168.63.129.16:80/machine/
    • Triggering command: /usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Co-authored-by: adlerjohn <3290375+adlerjohn@users.noreply.github.com>
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.

perf: Consider removing deepcopy

5 participants