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.

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

Jack Woollen's fix to optimize upb8.f routine #534

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Conversation

jbathegit
Copy link
Collaborator

Courtesy of @jack-woollen, and per the discussions in GSI/589 and GSI/642, this patch to subroutine upb8 optimizes performance for the majority of values which are encoded in fields of 32 bits or less.

@jbathegit jbathegit marked this pull request as ready for review November 2, 2023 19:23
@jbathegit
Copy link
Collaborator Author

BTW, I saw in the discussions how there's more to come w.r.t. improving the GSI timings, but in the meantime I don't see any reason not to go ahead and get this into the library baseline, since it's certainly a useful update :-)

@jack-woollen
Copy link
Contributor

Here's an interesting fact I stumbled on. As you know version 11.7.1 has the same ability to pack up numbers in 64 bits as v12.0.0. When I test 11.7.0, 11.7.1, and 12.0.0 against each other, both 11.7.1 and 12.0.0 show the same time increase against 11.7.0. Which means the imb8 blocks have nothing to do with the slowdown. It narrows the focus to what's slowing down 11.7.1, which is a much smaller change (I think) than 12.0.0. I can approve this if you want but there may be another change or two worth making sometime soon.

@jbathegit
Copy link
Collaborator Author

Yeah, that makes perfect sense Jack, given that the first issue you found was this one in upb8, and version 11.7.1 was indeed the first library release which included the capability to encode values larger than 32 bits (as noted in the Release Notes ;-)

So that was a good catch, and yes that should considerably narrow down the scope of the search!

In the meantime, yes please approve this so we can go ahead and at least get this upb8 upgrade into the code baseline, but I completely understand that there's likely more to come and which can always be added via one or more additional PRs at a later date.

@jbathegit jbathegit merged commit 6a6bf38 into develop Nov 2, 2023
6 checks passed
@jbathegit jbathegit deleted the jw-upb8fix branch November 2, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants