Skip to content

firestore: misc.ts: re-implemented compareUtf8Strings() to be even more simple and efficient #9143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jul 3, 2025

Re-implemented Firestore's compareUtf8Strings() function based on the greatly-improved algorithm from firebase/firebase-android-sdk#7098.

This algorithm has gone through a lot of churn recently due to performance issues with the naive implementation. This is (hopefully) the last PR to touch this function due to performance issues. The PR's leading up to this one are:

  1. FIX: sort strings in UTF-8 encoded byte order #8691 (slow, naive implementation)
  2. Revert the UTF-8 encoding in string sorting #8782 (revert the slow, naive impl)
  3. Fix: sort strings in UTF-8 encoded byte order with lazy encoding #8787 (faster, but complex impl)

@dconeybe dconeybe self-assigned this Jul 3, 2025
@dconeybe dconeybe requested review from a team as code owners July 3, 2025 19:41
Copy link

changeset-bot bot commented Jul 3, 2025

🦋 Changeset detected

Latest commit: da7185b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/firestore Patch
firebase Patch
@firebase/firestore-compat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    browser395 kB395 kB-356 B (-0.1%)
    main616 kB615 kB-555 B (-0.1%)
    module395 kB395 kB-356 B (-0.1%)
    react-native395 kB395 kB-357 B (-0.1%)
  • @firebase/firestore-lite

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    browser117 kB116 kB-347 B (-0.3%)
    main160 kB159 kB-608 B (-0.4%)
    module117 kB116 kB-347 B (-0.3%)
    react-native117 kB116 kB-349 B (-0.3%)
  • bundle

    15 size changes

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    firestore (CSI Auto Indexing Disable and Delete)291 kB290 kB-238 B (-0.1%)
    firestore (CSI Auto Indexing Enable)291 kB290 kB-238 B (-0.1%)
    firestore (Persistence)322 kB322 kB-201 B (-0.1%)
    firestore (Query Cursors)260 kB260 kB-201 B (-0.1%)
    firestore (Query)258 kB258 kB-201 B (-0.1%)
    firestore (Read data once)248 kB247 kB-201 B (-0.1%)
    firestore (Read Write w Persistence)342 kB342 kB-201 B (-0.1%)
    firestore (Realtime updates)248 kB248 kB-201 B (-0.1%)
    firestore (Transaction)227 kB227 kB-238 B (-0.1%)
    firestore (Write data)228 kB228 kB-238 B (-0.1%)
    firestore-lite (Query Cursors)111 kB111 kB-212 B (-0.2%)
    firestore-lite (Query)107 kB107 kB-212 B (-0.2%)
    firestore-lite (Read data once)82.9 kB82.7 kB-212 B (-0.3%)
    firestore-lite (Transaction)108 kB108 kB-212 B (-0.2%)
    firestore-lite (Write data)92.4 kB92.1 kB-212 B (-0.2%)

  • firebase

    TypeBase (ab5c2a0)Merge (f093d85)Diff
    firebase-compat.js807 kB807 kB-174 B (-0.0%)
    firebase-firestore-compat.js351 kB351 kB-174 B (-0.0%)
    firebase-firestore-lite.js140 kB139 kB-351 B (-0.3%)
    firebase-firestore.js459 kB458 kB-366 B (-0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/EyVnYwJDSz.html

@google-oss-bot
Copy link
Contributor

Size Analysis Report 1

This report is too large (864,920 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/WMaZTjQT7E.html

Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Thank you for the fix.

? primitiveComparator(leftChar, rightChar)
: isSurrogate(leftChar)
? 1
: -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultranit: I'd group the last expression in brackets, (isSurrogate(leftChar) ? 1 : -1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants