Skip to content

perf: do not use undefined for empty error array #4469

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: next
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Aug 8, 2025

Revert previously attempted micro-optimization as it no longer produces (never produced?) a true performance benefit.

@yaacovCR yaacovCR requested a review from a team as a code owner August 8, 2025 20:33
@yaacovCR yaacovCR changed the base branch from 16.x.x to next August 8, 2025 20:33
@yaacovCR yaacovCR marked this pull request as draft August 8, 2025 20:37
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Aug 8, 2025

odd, reverting both changes singly seems fine, but when benchmarking together, reverting does have a small perf hit (on our synthetic microbenchmarks)

@yaacovCR yaacovCR changed the title perf: do not use undefined for empty arrays perf: do not use undefined for empty error array Aug 8, 2025
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Aug 8, 2025

odd, reverting both changes singly seems fine, but when benchmarking together, reverting does have a small perf hit (on our synthetic microbenchmarks)

going to retain just the changes to the errors array rather than to the incrementalDataRecords array. the fix in terms of the latter is really to remove the entire idea of the GraphQLWrappedResult and instead filter incrementalDataRecords based on the bubbled null positions after execution, but this is something we will probably attempt only after incremental data delivery lands so that the implementation at this point can be closer to the spec.

@yaacovCR yaacovCR force-pushed the do-not-use-undefined branch from 7440c61 to 88c48c9 Compare August 8, 2025 20:54
@yaacovCR yaacovCR marked this pull request as ready for review August 8, 2025 20:54
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Aug 8, 2025

image

@yaacovCR yaacovCR added the PR: bug fix 🐞 requires increase of "patch" version number label Aug 8, 2025
@yaacovCR yaacovCR force-pushed the do-not-use-undefined branch from dc41871 to ef36972 Compare August 11, 2025 17:52
@yaacovCR yaacovCR force-pushed the do-not-use-undefined branch from ef36972 to d33fcf7 Compare August 11, 2025 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant