Skip to content

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
@yaacovCR yaacovCR merged commit 9e6c0e2 into graphql:next Aug 15, 2025
16 checks passed
@yaacovCR yaacovCR deleted the do-not-use-undefined branch August 15, 2025 06:49
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