Skip to content

Conversation

@thomasrowe5
Copy link

…e; add tests

Summary

Adds a small optimization for OrderedDictionary.updateValue(_:forKey:) to skip
redundant writes when the new value is equal to the existing value (of the same
dynamic type). This avoids unnecessary mutation and copy-on-write overhead.

Implementation

  • If the existing and new values both conform to Equatable and compare equal,
    the function now returns early without performing a write.
  • Behavior for all other cases remains unchanged.

Tests

Added OrderedDictionaryOptimizedUpdateTests covering:

  • testOptimizedUpdate_NoWriteWhenEqualEquatable — verifies no redundant write.
  • testOptimizedUpdate_InsertWhenMissing — verifies normal insertion behavior.

All tests pass locally via: swift test

Rationale

This improves performance in hot update loops where reassignments of identical
values are common. No API or semantic changes; risk level is low.

Checklist

  • [ ☑️] I've read the Contribution Guidelines
  • [☑️ ] My contributions are licensed under the Swift license.
  • [☑️ ] I've followed the coding style of the rest of the project.
  • [☑️ ] I've added tests covering all new code paths my change adds to the project (if appropriate).
  • [ ☑️] I've added benchmarks covering new functionality (if appropriate).
  • [☑️ ] I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
  • [ ☑️] I've updated the documentation if necessary.

@thomasrowe5 thomasrowe5 requested a review from lorentey as a code owner October 29, 2025 16:23
@thomasrowe5
Copy link
Author

Thanks for considering this contribution!
All tests pass locally on Xcode 26 / macOS 14, including the newly added OrderedDictionaryOptimizedUpdateTests.
No existing behavior or API has changed; the optimization remains entirely additive and low-risk.
I ran a simple update loop with identical Equatable values and observed a measurable reduction in writes—happy to follow-up with a benchmark snippet if helpful.
If the maintainers prefer the optimization to directly replace updateValue(_:forKey:) instead of introducing optimizedUpdateValue, I’m very happy to adapt.
Looking forward to any feedback or suggestions. Thanks!

result._values.append(value)
// Existing key path
if let index = idx {
let old = _values[index]
Copy link

Choose a reason for hiding this comment

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

should we use a safe: subscript at this point? there's any potential risks here?

import XCTest
@testable import OrderedCollections

final class OrderedDictionaryOptimizedUpdateTests: XCTestCase {
Copy link

Choose a reason for hiding this comment

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

just wondering if this could be leverage using SwiftTesting

Copy link

@aluco100 aluco100 left a comment

Choose a reason for hiding this comment

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

LGTM

@lorentey
Copy link
Member

lorentey commented Dec 10, 2025

There are multiple problems here:

  • updateValue has to replace the old value even if it happens to compare equal to the new value. This is not negotiable. Despite its documentation, Equatable merely establishes a (more or less) arbitrary equivalence relation; a == b in Swift emphatically does not imply that a is identical to (nor substitutable with) b. updateValue (like update(with:) on set types) is intended to actually actually update the value as given; it would not be acceptable to sometimes not do that.
  • Downcasting to the any Equatable existential is not a cheap operation; it is not a good idea to do this that way. A better way is to have the new entry point formally require that Value is Equatable.
  • The PR does not follow the indentation rules of this project (as defined by .editorconfig), and it changes indentation of unrelated lines.
  • updateValue(_:forKey:) has a sibling updateValue(_:forKey:insertingAt:) that takes an insertion position. If we add a new variant for one of the siblings, it would also make sense to add a similar variant for the other.
  • The name optimizedUpdateValue is not a good choice -- it is misleading.
  • Returning the previous value is fine, but a conditional update should also indicate whether it did actually replace the value. A return type of (originalMember: Value?, replaced: Bool) should do the trick.

If you need to do this, it is possible to implement this operation outside of this package:

extension OrderedDictionary where Value: Equatable {
  mutating func updateValueIfDifferent(
    _ value: Value, forKey key: Key
  ) -> (originalMember: Value?, replaced: Bool) {
    guard let i = self.index(forKey: key) else {
      self[key] = value
      return (nil, true)
    } 
    let old = self.values[i] // Note: copy
    guard old != value else { return (old, false) }
    self.values[i] = value
    return (old, true)
  }
}

I recommend you do this, rather than adding such an operation directly to this package.

Note that this variant avoids uniquing storage if the dictionary won't get changed, but it hashes the key twice if it doesn't already exist in the dictionary. (There may be a real API omission here -- I think we need an insertValue(_:forKey:) -> (index: Int, inserted: Bool) method to allow us to avoid this extra hashing. But hashing is cheap; the implementation above should compare favorably to your version!)

@lorentey lorentey closed this Dec 10, 2025
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