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

Optimize bit handling in archive_compressor_xz_init_stream #1990

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Optimize bit handling in archive_compressor_xz_init_stream #1990

wants to merge 1 commit into from

Conversation

AtariDreams
Copy link
Contributor

@AtariDreams AtariDreams commented Oct 11, 2023 •
edited

Add intrin.h for _BitScanReverse. This makes the operations being done much clearer and also faster as well.

evelikov
evelikov suggested changes Oct 12, 2023
View reviewed changes
libarchive/archive_check_magic.c Outdated Show resolved Hide resolved
libarchive/archive_windows.h Show resolved Hide resolved
libarchive/archive_write_add_filter_xz.c Outdated Show resolved Hide resolved
libarchive/archive_write_add_filter_xz.c Show resolved Hide resolved
@AtariDreams
Copy link
Contributor Author

@evelikov Done!

evelikov
evelikov approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@evelikov evelikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even without the optimisation bits, the commit does a good job and fixing the respective types.

The spec has a lot of "undefined" mentions when dealing with signed ints, shifts and alike.

Thanks o/

@AtariDreams AtariDreams changed the title Optimize bit handling in places where we can Optimize bit handling in archive_compressor_xz_init_stream Oct 22, 2023
@evelikov
Copy link
Contributor

@AtariDreams the extra changes that got squashed a few minutes ago do not belong to this MR.

@AtariDreams
Copy link
Contributor Author

@AtariDreams the extra changes that got squashed a few minutes ago do not belong to this MR.

My bad

kientzle
kientzle reviewed Dec 16, 2023
View reviewed changes
libarchive/archive_read_support_filter_gzip.c
@@ -209,7 +209,7 @@ peek_at_header(struct archive_read_filter *filter, int *pbits,
}

/* Optional header CRC */
if ((header_flags & 2)) {
if (header_flags & 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra parentheses are needed here. That's used by several C compilers to indicate that you're doing a boolean check.

@kientzle
Copy link
Contributor

As @evelikov mentioned, we'd appreciate any measurements that would show the performance improvement here.

Generally, we prefer to not have different code paths for different platforms unless it's necessary or provides significant performance gains. Having lots of #if sections makes code harder to read; having different code on different platforms makes it harder to accurately test.

@AtariDreams
Optimize bit handling in archive_compressor_xz_init_stream
7fc99c8 
Add intrin.h for _BitScanReverse. This makes the operations being done much clearer and also faster as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kientzle kientzle kientzle left review comments

@evelikov evelikov evelikov approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AtariDreams @evelikov @kientzle