Optimize multidimensional arrays, 32-bit read/write, FastMap.

[TheLight-233]

Optimize multidimensional arrays, 32-bit read/write, FastMap.

[github-actions]

Code Review for PR #169: Optimize multidimensional arrays, 32-bit read/write, FastMap

Summary

This PR introduces significant performance optimizations including a new FastMap implementation, 32-bit platform optimizations, and improved multidimensional array serialization. Overall, the changes demonstrate strong performance awareness, but there are several issues that should be addressed before merging.

🔴 Critical Issues

1. FastMap.cs - Memory Alignment and Safety Issues

Unaligned Access Concerns (Line 23-29)

[StructLayout(LayoutKind.Sequential, Pack = 1)]
private struct Entry
{
    public uint HashCode;      // Offset 0, aligned
    public bool IsOccupied;    // Offset 4, aligned
    public TKey Key;           // Offset 5, UNALIGNED!
    public TValue Value;       // Varies, UNALIGNED!
}

The Pack = 1 attribute eliminates padding, which means Key starts at an unaligned offset (5 bytes). This can cause:

• Performance degradation on x86/x64 (slower unaligned access)
• Crashes or exceptions on ARM and other strict-alignment architectures
• Potential data corruption with concurrent access

Recommendation: Either remove Pack = 1 or reorder fields to ensure proper alignment:

[StructLayout(LayoutKind.Sequential)]
private struct Entry
{
    public bool IsOccupied;    // 1 byte + 3 bytes padding
    public uint HashCode;      // 4 bytes, aligned at offset 4
    public TKey Key;           // Properly aligned at offset 8
    public TValue Value;
}

Memory Leak in Remove() (Line 431-457)

public bool Remove(TKey key)
{
    // ...
    entry1.IsOccupied = false;  // ❌ Key and Value not cleared!
    _count--;
    _version++;
    return true;
}

Setting IsOccupied = false without clearing Key and Value prevents garbage collection of referenced objects. This is a memory leak.

Recommendation: Clear the entry completely:

entry1 = default;
entry1.IsOccupied = false;

Infinite Recursion Risk (Line 347)

if (!TryAdd(key, default))
    throw new InvalidOperationException("Failed to add key to dictionary");
return ref GetValueRefOrAddDefault(key, out exists);  // ❌ Recursive call

If TryAdd succeeds but the key can't be found (due to a bug or race condition), this creates infinite recursion.

Recommendation: Get the value reference directly after successful TryAdd:

if (!TryAdd(key, default))
    throw new InvalidOperationException("Failed to add key to dictionary");
// Return reference to the just-added key
var hashCode = key.GetHashCode();
hashCode ^= hashCode >> 16;
var index1 = hashCode & _capacityMask;
ref Entry entry1 = ref _table1[index1];
if (entry1.HashCode == hashCode && KeyComparer.Equals(entry1.Key, key))
{
    exists = false;
    return ref entry1.Value;
}
// Check table2...

2. ArrayGenerator.cs - Integer Overflow Risk

Total Elements Calculation (Lines 143-149)

int totalElements = len0 * len1 * len2;  // ❌ Can overflow!

With large array dimensions, this multiplication can silently overflow, leading to incorrect serialization or buffer overflows.

Recommendation: Use checked arithmetic:

int totalElements;
try
{
    checked
    {
        totalElements = len0;
        for (int i = 1; i < rank; i++)
            totalElements *= len{i};
    }
}
catch (OverflowException)
{
    throw new InvalidOperationException("Array dimensions too large for serialization");
}

🟡 Medium Priority Issues

3. FastMap.cs - Missing Capacity Overflow Check

CalculateCapacity() (Lines 101-111)

private static int CalculateCapacity(int capacity)
{
    if (capacity < MinCapacity) return MinCapacity;
    capacity--;
    capacity |= capacity >> 1;
    // ... bit manipulation
    return capacity + 1;  // ❌ Can overflow to negative!
}

With very large input values (e.g., int.MaxValue), the final capacity + 1 can overflow to a negative number or int.MinValue.

Recommendation: Add overflow check:

if (capacity > (1 << 30))
    throw new ArgumentOutOfRangeException(nameof(capacity), "Capacity too large");

4. Reader.cs/Writer.cs - Platform Detection Overhead

The TypeCollector.Is64Bit check is performed on every read/write operation, adding branching overhead. While this is acceptable, consider:

Recommendation: Use compile-time checks where possible or JIT-level optimization hints:

[MethodImpl(MethodImplOptions.AggressiveOptimization)]

5. ArrayGenerator.cs - Confusing Naming

CachedSerializer.SerializePolymorphic (Lines 114, 263)

The method name SerializePolymorphic is used in the monomorphic fast path, which is confusing.

Recommendation: Rename to CachedSerializer<T>.Serialize or add a comment explaining the naming.

🟢 Positive Observations

1. Excellent Performance Awareness: The cuckoo hashing implementation in FastMap is well-designed for cache locality
2. 32-bit Platform Support: Thoughtful handling of 32-bit platforms with safe memory operations
3. Code Reduction: ArrayGenerator went from 196 to 114 lines while adding functionality - excellent refactoring
4. Row-Major Order: Proper use of row-major iteration for cache-friendly multidimensional array access
5. Monomorphic Optimization: Smart use of sealed type checks to avoid delegate allocation
6. Version Tracking: Proper collection modification detection in FastMap enumerator
7. Comprehensive Test Coverage: Multiple overloads (out, ref) with proper null handling

📋 Minor Issues & Suggestions

Code Quality

• FastMap Line 85: Copy constructor takes in parameter but source is a reference type - in has no effect
• ArrayGenerator Line 165: Comment "space locality" could be more specific - "cache-friendly row-major order"
• Writer.cs Line 100-123: Consider extracting the size-based switch to a separate method for readability

Security

• No obvious security issues, but ensure Reader validates all length fields to prevent buffer overruns in untrusted scenarios

Test Coverage

• Consider adding tests for:
  • FastMap with large capacities near int.MaxValue
  • Multidimensional arrays with maximum dimensions
  • 32-bit platform-specific edge cases
  • FastMap collision scenarios (MaxKickCount exceeded)

❓ Questions for Author

1. Missing NinoMarshal.cs: The PR description mentions NinoMarshal.cs with 114 additions, but this file doesn't appear to be in the repository. Is this file missing from the PR, or is it generated code?

2. Performance Benchmarks: Have you run benchmarks comparing FastMap vs Dictionary, and the new multidimensional array serialization vs the old implementation?

3. 32-bit Testing: Have these changes been tested on actual 32-bit platforms or ARM devices?

4. Breaking Changes: Are there any breaking changes in the serialization format that would affect existing serialized data?

🎯 Recommendation

Request Changes - The critical memory alignment and safety issues in FastMap.cs should be addressed before merging. The integer overflow risk in ArrayGenerator.cs also needs attention. Once these are fixed, this will be an excellent performance improvement.

Priority Action Items

1. ✅ Fix FastMap Entry struct alignment issues
2. ✅ Clear Key/Value in FastMap.Remove()
3. ✅ Fix infinite recursion risk in GetValueRefOrAddDefault
4. ✅ Add overflow checks to ArrayGenerator totalElements calculation
5. ⚠️ Clarify missing NinoMarshal.cs file
6. ⚠️ Add capacity overflow check in CalculateCapacity()

---

Overall, this is ambitious and performance-conscious work. The core ideas are sound, but the implementation needs refinement for production use. Looking forward to seeing these issues addressed!

[github-actions]

Code Review for PR #169: Optimize multidimensional arrays, 32-bit read/write, FastMap

Thank you for this comprehensive optimization PR! This is a substantial improvement to Nino's performance characteristics. Below is my detailed review:

🎯 Overall Assessment

Verdict: Strong contribution with excellent performance optimizations. A few minor suggestions for consideration.

Summary: This PR adds 1,035 lines and removes 223 lines across 5 files, focusing on:

1. Complete FastMap implementation (new cuckoo hash table)
2. New NinoMarshal utility class
3. Optimized 32-bit platform support in Reader/Writer
4. Enhanced multidimensional array serialization

---

✅ Code Quality & Best Practices

Strengths:

• Excellent documentation: FastMap includes clear comments explaining design decisions (e.g., Pack=1 trade-offs, cache locality)
• Proper use of modern C# features: Aggressive inlining, ref returns, spans, and conditional compilation
• Performance-conscious design: Cache-friendly memory layouts, ref-based iteration, version tracking for collection modification detection
• Comprehensive array handling: Supports 1D, jagged, and true multidimensional arrays with rank validation

Suggestions:

1. FastMap.cs - Missing null checks in hot paths (lines 61, 66)

// Current code at lines 61-62:
if (entry1.HashCode == hashCode && KeyComparer.Equals(entry1.Key, key))

Issue: If entry1.IsOccupied is false, the Key field may contain default/uninitialized values, potentially causing unnecessary equality comparisons.

Recommendation: Add occupancy check before key comparison:

if (entry1.IsOccupied && entry1.HashCode == hashCode && KeyComparer.Equals(entry1.Key, key))
    return ref entry1.Value;

2. FastMap.cs - Potential infinite recursion (line 347)

public ref TValue GetValueRefOrAddDefault(TKey key, out bool exists)
{
    // ... code ...
    else
    {
        if (!TryAdd(key, default))
            throw new InvalidOperationException("Failed to add key to dictionary");
        return ref GetValueRefOrAddDefault(key, out exists); // ⚠️ Recursion
    }
}

Issue: If TryAdd succeeds but returns to this branch again, infinite recursion could occur (unlikely but theoretically possible).

Recommendation: After successful TryAdd, directly retrieve the reference:

if (!TryAdd(key, default))
    throw new InvalidOperationException("Failed to add key to dictionary");
// Key now exists, retrieve it directly
var hashCode = key.GetHashCode();
hashCode ^= hashCode >> 16;
var index1 = hashCode & _capacityMask;
ref Entry entry1 = ref _table1[index1];
if (entry1.IsOccupied && entry1.HashCode == hashCode && KeyComparer.Equals(entry1.Key, key))
{
    exists = false;
    return ref entry1.Value;
}
// Check table2...

3. ArrayGenerator.cs - Code clarity

The monomorphic path optimization is excellent, but consider adding XML documentation comments to explain when CachedSerializer vs direct serialization is used.

---

🐛 Potential Bugs

Critical:

None identified - The code appears functionally sound.

Minor:

FastMap.cs line 147-148:

var hashCode = key.GetHashCode();
hashCode ^= hashCode >> 16;

Observation: This duplicates the TransformHashCode method.

Recommendation: Use the helper consistently:

var hashCode = (uint)TransformHashCode(key.GetHashCode());

Writer.cs lines 101-123:

The 32-bit safe path uses a switch on size but doesn't handle all scenarios explicitly.

Question: Should there be validation that size is reasonable (e.g., < 1MB) to prevent abuse?

---

⚡ Performance Considerations

Excellent Optimizations:

1. FastMap cuckoo hashing: O(1) average case with low cache misses
2. 32-bit platform improvements: Eliminates unaligned 8-byte writes that could cause crashes on ARM32
3. Array serialization:
   • Fast path for unmanaged types (direct memory copy)
   • Monomorphic path for sealed/struct NinoTypes (avoids delegate allocation)
   • Ref-based iteration eliminates bounds checks
   • Space-locality-aware traversal for multidimensional arrays

Suggestions:

Reader.cs/Writer.cs - Conditional compilation overhead

Lines using if (TypeCollector.Is64Bit) branches:

• These are runtime checks that could potentially be optimized with #if directives if the target platform is known at compile time
• However, the JIT should eliminate these branches via constant folding, so this is likely already optimal

ArrayGenerator.cs - Memory allocation

For very large multidimensional arrays, consider:

• Streaming serialization (chunked writes) to reduce memory pressure
• Progress callbacks for long-running operations
• However, this may be out of scope for this PR

---

🔒 Security Concerns

Low Risk - No critical issues found

Input Validation:

1. ArrayGenerator.cs lines 302-305: Rank mismatch throws exception ✅
2. FastMap capacity: Constructor validates capacity >= 0 ✅
3. Reader string methods: UTF-8 validation included ✅

Potential DoS consideration:

• FastMap resize: Unlimited growth could exhaust memory if attacker controls input
  • Mitigation: Consider adding max capacity limits in production scenarios
  • Note: This is a design decision, not a bug. Document expected usage patterns.

Memory safety:

• Extensive use of unsafe code is appropriate for a serialization library
• All unsafe blocks appear correctly bounded

---

🧪 Test Coverage

⚠️ Test Coverage Analysis

Based on the PR diff, I don't see test files included. Strongly recommend adding tests for:

High Priority:

1. FastMap:

   - Add/Get/Remove operations
   - Collision handling (cuckoo evictions)
   - Resize behavior when capacity exceeded
   - Concurrent access (if supported)
   - Edge cases: empty map, single element, max capacity

2. Multidimensional arrays:

   - 2D, 3D, and higher rank arrays
   - Empty arrays and arrays with zero-length dimensions
   - Rank mismatch scenarios (verify exception)
   - Mixed unmanaged/managed element types

3. 32-bit platform compatibility:

   - Test on actual 32-bit runtime (or emulated)
   - Verify no alignment faults occur
   - Benchmark performance parity with 64-bit where possible

Medium Priority:

4. NinoMarshal (new file - needs baseline tests)
5. Reader/Writer optimizations (regression tests)

Recommendation:

Add integration tests and benchmarks before merging to ensure:

• No performance regressions
• Correctness across all platforms (.NET Framework 2.1, .NET 6+, Unity)

---

📋 Additional Notes

Documentation:

• Consider adding migration guide if FastMap replaces existing dictionary usage
• Performance benchmarks would be valuable (before/after comparisons)
• Update README or docs with FastMap usage examples

Code Organization:

• Excellent use of #if directives for platform-specific code
• Consistent naming conventions followed
• [MethodImpl] attributes properly applied

Compatibility:

• Multi-targeting appears correct (NET5_0_OR_GREATER, NET6_0_OR_GREATER, etc.)
• Unity compatibility maintained

---

🎯 Conclusion

This is a high-quality PR that demonstrates deep understanding of:

• Low-level .NET performance optimization
• Platform-specific concerns (32-bit vs 64-bit)
• Serialization library design patterns

Before Merging:

1. ✅ Address the FastMap indexer occupancy check (critical for correctness)
2. ✅ Fix GetValueRefOrAddDefault recursion (low probability, but important)
3. ⚠️ Add comprehensive test coverage
4. 📝 Consider adding benchmarks to demonstrate improvements

Recommendation:

Approve with minor revisions - Once the occupancy checks are added and tests are included, this will be an excellent addition to Nino.

Great work @TheLight-233! 🚀

---

Review generated with assistance from Claude Code

[github-actions]

Code Review for PR #169: Optimize multidimensional arrays, 32-bit read/write, FastMap

Thank you for your contribution! I've reviewed the changes in this PR based on the repository code and test coverage.

Positive Aspects

1. Performance Optimizations: The PR adds valuable optimizations for multidimensional array serialization/deserialization and 32-bit platform support.

2. Comprehensive Test Coverage: The existing test suite includes extensive coverage for multi-dimensional arrays, jagged arrays, edge cases, and complex scenarios.

3. Code Structure: The generated code in ArrayGenerator.cs follows consistent patterns with proper use of AggressiveInlining attributes, null checks, and memory-efficient iteration.

Areas for Review

1. New File: NinoMarshal.cs (114 lines)

• Question: What functionality does NinoMarshal provide? Is it related to the 32-bit optimizations?
• Recommendation: Please provide XML documentation comments for public APIs.
• Consider: Ensure no code duplication with existing Writer/Reader functionality.

2. ArrayGenerator.cs Modifications

Monomorphic Fast Path: The canUseMonomorphicPath optimization is only applied to 1D arrays. Is there a reason multi-dimensional arrays can't benefit from this?

32-bit Platform Considerations: Are the new optimizations tested on both 32-bit and 64-bit platforms? Have you verified unaligned write safety on 32-bit platforms?

WEAK_VERSION_TOLERANCE Handling: Good optimization - unmanaged types don't need version tolerance overhead.

Space Locality: The row-major iteration order naturally provides good cache locality. Good practice!

3. Potential Issues

Array Rank Mismatch Error: Verify string interpolation escaping is correct in generated exception messages.

Recursive GetValueRefOrAddDefault (FastMap.cs:347): Could potentially cause infinite recursion. Consider adding a safeguard or comment.

FastMap Pack=1: May cause unaligned memory access on ARM. Verify this works on Unity mobile targets.

Testing Recommendations

1. Add tests for NinoMarshal.cs functionality
2. Verify 32-bit specific optimizations with tests on 32-bit runtime
3. Include performance benchmarks showing improvements
4. Test edge cases: very large arrays, zero dimensions, memory pressure

Security Considerations

• Array bounds: Proper GetLength() checks ✅
• Null handling: In place ✅
• Exception handling: Appropriate ✅
• Integer overflow: When calculating totalElements for large multi-dimensional arrays, consider using checked arithmetic or validation to prevent overflow from malicious/corrupted data.

Documentation

• Add XML documentation for NinoMarshal.cs public APIs
• Document the monomorphic optimization strategy
• Update CHANGELOG with performance improvements

Overall Assessment

Solid performance optimization with well-structured code and good test coverage. Address questions around NinoMarshal.cs purpose, 32-bit testing, integer overflow protection, and provide benchmark results.

---

Review generated by Claude Code

[github-actions]

Pull Request Review: Optimize multidimensional arrays, 32-bit read/write, FastMap

Thank you for this comprehensive performance optimization PR! I have conducted a thorough review of the changes.

---

✅ Strengths

1. Excellent 32-bit Platform Safety (Writer.cs:89-127)

The refactored UnsafeWrite method shows excellent awareness of alignment issues on 32-bit platforms with proper size-aware switch statement and safe memory copy for 8+ byte values.

2. High-Performance FastMap Implementation (FastMap.cs)

• Pack=1 struct layout improves cache utilization
• Dual hash tables with cuckoo hashing
• Ref-based access enables zero-copy operations
• Version tracking prevents modification during enumeration

3. Multidimensional Array Support (ArrayGenerator.cs)

• Row-major order traversal for cache locality
• Monomorphic path optimization avoids delegate allocation
• Fast path detection for unmanaged types
• Proper rank validation during deserialization

4. Performance Optimizations (Reader.cs:394-410)

• Element size caching reduces redundant calls
• Improved array allocation with NET5+ optimizations

---

⚠️ Critical Issues

1. Missing NinoMarshal.cs File

The PR mentions this file (114 additions) but it is not present in the codebase. Please verify if this should be included.

2. FastMap: Integer Overflow Risk (FastMap.cs:510)

private void Resize() => Resize(_capacity * 2);

When _capacity approaches int.MaxValue/2, multiplication can overflow causing negative capacity. Consider: _capacity >= int.MaxValue / 2 ? int.MaxValue : _capacity * 2

3. FastMap: Unsafe Dispose Pattern (FastMap.cs:499-507)

No disposal guard - multiple Dispose() calls or usage after disposal can cause NullReferenceException. IDisposable should be idempotent with _disposed flag.

---

🟡 Moderate Issues

4. Unhelpful Exception Messages (FastMap.cs:68, 141, 346, 393)

Generic exceptions lack context. Include key information for debugging.

5. Hash Collision Risk (FastMap.cs:47-50, 147-148, 188-189)

Hash transformation duplicated with variations. Use TransformHashCode() consistently everywhere.

6. Copy Constructor Issue (FastMap.cs:85-98)

Copying version number from source does not make semantic sense. New instance should start with version 0.

7. IsCreated Property Misleading (FastMap.cs:44)

Returns false after Dispose() but object still exists. Consider renaming or adding disposal guards.

---

🔒 Security Concerns

11. Denial of Service Risk: Array Allocation

Deserialization allocates arrays based on serialized sizes without limits. An attacker could serialize length=int.MaxValue causing OutOfMemoryException. Add configurable size limits (e.g., MaxArrayLength = 100_000_000).

12. MaxKickCount Configuration

In adversarial scenarios with hash collision attacks, fixed MaxKickCount=16 could be exploited. Consider making configurable or documenting security implications.

---

🧪 Test Coverage Concerns

Missing tests for:

1. 32-bit platform code paths
2. FastMap edge cases (collisions, resize near int.MaxValue, dispose scenarios)
3. Multidimensional arrays (rank > 2, large arrays, rank mismatch)
4. DoS scenarios (extremely large arrays)

---

📊 Performance & Documentation

Please include:

• Benchmark results (1D arrays, multidimensional, 32-bit vs 64-bit, FastMap vs Dictionary)
• XML documentation for FastMap (when to use, performance, thread-safety)
• Wire format documentation for multidimensional arrays

---

✅ Summary

Must Fix:

• Clarify NinoMarshal.cs status
• Fix integer overflow in FastMap.Resize()
• Implement proper Dispose pattern

Should Fix:

• Consistent hash transformation usage
• Fix version copying in copy constructor
• Add array size limits to prevent DoS

Nice to Have:

• Comprehensive test coverage
• Benchmark results
• Improved exception messages
• XML documentation

Overall: Strong performance-focused PR with valuable optimizations. The multidimensional array support and 32-bit safety are excellent, but critical issues need attention before merging.

Review completed according to WORKFLOW.md guidelines