Skip to content

Unbreak delta reporting in benchmarks #61236

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

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented Sep 21, 2022

build-script -B has been broken for me for a while. I finally tracked it down to this bit of code.

The logic here was apparently intended to omit literal zeros from deltas to save a few bytes, but it instead drops all zeros from all columns. Remove the condition that drops zeros in order to avoid confusing the many scripts that consume this data.

Alternatives Considered

I'm probably going to entirely drop the delta form in an upcoming PR, so I didn't think it was worthwhile to do something more complex, such as:

  • Fixing this logic to only omit zeros from actual delta columns

  • Rewriting all the client scripts to treat any empty column as zero

The logic here was apparently intended to omit literal zeros from deltas
to save a few bytes, but it instead drops all zeros from all columns.
Remove the condition that drops zeros in order to avoid confusing
the many scripts that consume this data.

Alternatives Considered

I'm probably going to entirely drop the delta form in an upcoming
PR, so I didn't think it was worthwhile to do something more complex,
such as:

* Fixing this logic to only omit zeros from actual delta columns

* Rewriting all the client scripts to treat any empty column as zero
@tbkka
Copy link
Contributor Author

tbkka commented Sep 21, 2022

@swift-ci Please test

@tbkka
Copy link
Contributor Author

tbkka commented Sep 21, 2022

@swift-ci Please benchmark

@tbkka tbkka merged commit 48c1931 into swiftlang:main Sep 22, 2022
@tbkka tbkka deleted the tbkka-unbreak-delta-benchmarks branch August 1, 2024 16:37
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.

1 participant