Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Firestore: Choose DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT value based on the browser, rather than hardcoding 8 #7929

Merged
merged 2 commits into from
Jan 8, 2024
Merged

Firestore: Choose DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT value based on the browser, rather than hardcoding 8 #7929

merged 2 commits into from
Jan 8, 2024

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jan 8, 2024 •
edited
Loading

A previous PR, #7608, hardcoded the value of DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT to 8. At the time, this was the "worst case" value, but penalized many platform/browser combinations that would experience performance benefits from a lower value.

Since then, an experiment was performed to calculate a more reasonable value. These optimal values were:

  1. 8 for Safari
  2. 6 for Android
  3. 4 for everything else (e.g. desktop browsers)

This PR changes the hardcoded value of 8 to one of the values above, depending on the runtime environment.

NOTE: This PR accidentally increased the code bundle size by 6.5 kB starting in v10.7.2. This bundle size regression was fixed by #8178.

@dconeybe dconeybe self-assigned this Jan 8, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: b5424e3

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

This PR includes changesets to release 3 packages
Name Type
firebase Patch
@firebase/firestore 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

@dconeybe dconeybe changed the title Firestore: query_engine.ts: choose DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT based on the browser, rather than hardcoding 8 Firestore: Choose DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT value based on the browser, rather than hardcoding 8 Jan 8, 2024
@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (6ea51fb)Merge (8432fe7)Diff
    browser375 kB375 kB+121 B (+0.0%)
    esm5360 kB360 kB+31 B (+0.0%)
    main577 kB577 kB+104 B (+0.0%)
    module375 kB375 kB+121 B (+0.0%)
    react-native375 kB375 kB+121 B (+0.0%)

  • bundle

    TypeBase (6ea51fb)Merge (8432fe7)Diff
    firestore (CSI Auto Indexing Disable and Delete)268 kB268 kB+43 B (+0.0%)
    firestore (CSI Auto Indexing Enable)268 kB268 kB+43 B (+0.0%)
    firestore (Persistence)303 kB303 kB+43 B (+0.0%)
    firestore (Query Cursors)238 kB246 kB+7.16 kB (+3.0%)
    firestore (Query)236 kB243 kB+7.16 kB (+3.0%)
    firestore (Read data once)224 kB231 kB+7.16 kB (+3.2%)
    firestore (Read Write w Persistence)321 kB321 kB+43 B (+0.0%)
    firestore (Realtime updates)226 kB234 kB+7.16 kB (+3.2%)
    firestore (Transaction)204 kB211 kB+7.16 kB (+3.5%)
    firestore (Write data)204 kB211 kB+7.16 kB (+3.5%)

  • firebase

    TypeBase (6ea51fb)Merge (8432fe7)Diff
    firebase-compat.js779 kB779 kB+47 B (+0.0%)
    firebase-firestore-compat.js341 kB341 kB+20 B (+0.0%)
    firebase-firestore.js434 kB434 kB+121 B (+0.0%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

Size Analysis Report 1

This report is too large (482,330 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/2vOKKmMjmq.html

@dconeybe dconeybe marked this pull request as ready for review January 8, 2024 21:29
@dconeybe dconeybe requested review from a team as code owners January 8, 2024 21:29
cherylEnkidu
cherylEnkidu approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@cherylEnkidu cherylEnkidu 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 !

@dconeybe dconeybe merged commit d7ace80 into master Jan 8, 2024
44 checks passed
@dconeybe dconeybe deleted the dconeybe/RelativeIndexReadCostFromExperimentRatherThanHardcoding8 branch January 8, 2024 22:22
@google-oss-bot google-oss-bot mentioned this pull request Jan 16, 2024
Merged
@dconeybe dconeybe mentioned this pull request Jan 19, 2024
Merged
DellaBitta pushed a commit that referenced this pull request Jan 19, 2024
@Yonom
Fix isSafari() throwing on React Native (fixes #7962) (#7963)
434f841 
In #7929 some logic was added to Firestore to check for the browser being Safari. This new check for isSafari() unexpectedly threw an exception in React Native. This PR fixes the exception by explicitly checking for an object not being undefined before using it.

Fixes #7962
@firebase firebase locked and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cherylEnkidu cherylEnkidu cherylEnkidu approved these changes

Assignees

@dconeybe dconeybe

@cherylEnkidu cherylEnkidu

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dconeybe @google-oss-bot @cherylEnkidu