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

Avoid a lot of string copies in ext-phar #17240

Closed
wants to merge 2 commits into from
Closed

Avoid a lot of string copies in ext-phar #17240

wants to merge 2 commits into from
+309 −329

Conversation

nielsdos
Copy link
Member

This is only the first batch, but it's large enough already to warrant its own PR.

@Girgias
Copy link
Member

Girgias commented Dec 23, 2024

Feel free to merge the first commit already.

Girgias
Girgias reviewed Dec 23, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I left some initial comments :)

ext/phar/stream.c Show resolved Hide resolved
ext/phar/stream.c Show resolved Hide resolved
ext/phar/stream.c Outdated Show resolved Hide resolved
ext/phar/phar_object.c Outdated Show resolved Hide resolved
ext/phar/tar.c Outdated Show resolved Hide resolved
ext/phar/tar.c
if (phar_metadata_tracker_has_data(&entry->phar->metadata_tracker, entry->phar->is_persistent)) {
efree(metadata);
return FAILURE;
}
entry->phar->metadata_tracker = entry->metadata_tracker;
entry->metadata_tracker.str = NULL;
ZVAL_UNDEF(&entry->metadata_tracker.val);
} else if (entry->filename_len >= sizeof(".phar/.metadata/") + sizeof("/.metadata.bin") - 1 && NULL != (mentry = zend_hash_str_find_ptr(&(entry->phar->manifest), entry->filename + sizeof(".phar/.metadata/") - 1, entry->filename_len - (sizeof("/.metadata.bin") - 1 + sizeof(".phar/.metadata/") - 1)))) {
} else if (ZSTR_LEN(entry->filename) >= sizeof(".phar/.metadata/") + sizeof("/.metadata.bin") - 1 && NULL != (mentry = zend_hash_str_find_ptr(&entry->phar->manifest, ZSTR_VAL(entry->filename) + sizeof(".phar/.metadata/") - 1, ZSTR_LEN(entry->filename) - (sizeof("/.metadata.bin") - 1 + sizeof(".phar/.metadata/") - 1)))) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use zend_string_starts_with_literal here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is a hash lookup

Copy link
Member

Choose a reason for hiding this comment

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

same comment as the previous one regarding the leading/trailing /

ext/phar/tar.c Outdated
Comment on lines 913 to 915
if (entry->filename_len >= sizeof(".phar/.metadata") && !memcmp(entry->filename, ".phar/.metadata", sizeof(".phar/.metadata")-1)) {
if (entry->filename_len == sizeof(".phar/.metadata.bin")-1 && !memcmp(entry->filename, ".phar/.metadata.bin", sizeof(".phar/.metadata.bin")-1)) {
if (ZSTR_LEN(entry->filename) >= sizeof(".phar/.metadata") && !memcmp(ZSTR_VAL(entry->filename), ".phar/.metadata", sizeof(".phar/.metadata")-1)) {
if (ZSTR_LEN(entry->filename) == sizeof(".phar/.metadata.bin")-1 && !memcmp(ZSTR_VAL(entry->filename), ".phar/.metadata.bin", sizeof(".phar/.metadata.bin")-1)) {
return phar_tar_setmetadata(&entry->phar->metadata_tracker, entry, error);
}
/* search for the file this metadata entry references */
if (entry->filename_len >= sizeof(".phar/.metadata/") + sizeof("/.metadata.bin") - 1 && !zend_hash_str_exists(&(entry->phar->manifest), entry->filename + sizeof(".phar/.metadata/") - 1, entry->filename_len - (sizeof("/.metadata.bin") - 1 + sizeof(".phar/.metadata/") - 1))) {
if (ZSTR_LEN(entry->filename) >= sizeof(".phar/.metadata/") + sizeof("/.metadata.bin") - 1 && !zend_hash_str_exists(&entry->phar->manifest, ZSTR_VAL(entry->filename) + sizeof(".phar/.metadata/") - 1, ZSTR_LEN(entry->filename) - (sizeof("/.metadata.bin") - 1 + sizeof(".phar/.metadata/") - 1))) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto zend string equals/start with APIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is a hash lookup

Copy link
Member

Choose a reason for hiding this comment

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

Okay, this is confusing, also isn't there a bug with:

ZSTR_LEN(entry->filename) >= sizeof(".phar/.metadata/") + sizeof("/.metadata.bin") - 1

as it has the / both as a trailing and leading character?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I believe this is for: .phar/.metadata/FILENAME_HERE/.metadata.bin in which case it makes sense. And the fact that we only do -1 instead of -2 for the 2 sizeofs is because it enforces that FILENAME_HERE is >= 1 character in size.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, this is really unintuitive >.> but nothing new with Phar

ext/phar/tar.c
@@ -1133,13 +1124,13 @@ void phar_tar_flush(phar_archive_data *phar, zend_string *user_stub, bool is_def
} else {
phar_entry_info newentry = {0};

newentry.filename = estrndup(".phar/.metadata.bin", sizeof(".phar/.metadata.bin")-1);
newentry.filename_len = sizeof(".phar/.metadata.bin")-1;
newentry.filename = ZSTR_INIT_LITERAL(".phar/.metadata.bin", false);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to intern those at startup of the extension? (Obviously for a future PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

We could

ext/phar/zip.c Outdated Show resolved Hide resolved
ext/phar/zip.c Outdated Show resolved Hide resolved
nielsdos added a commit that referenced this pull request Dec 26, 2024
@nielsdos
Use format string to cut off filename instead of duplicating memory
b09ed9a 
Split off from GH-17240.
@nielsdos
Transform phar_entry_info filename to zend_string
cb609e3 
The contents of the string are copied many times, especially in hash
tables. Avoid all this work by using zend_string in the first place.
@nielsdos
Use zend_string_{equals,startswith} helpers
f079964
@staabm
Copy link
Contributor

staabm commented Jan 3, 2025

do you expect this will have a positive impact on .phar based CLI applications, e.g. PHPStan in some form?

@nielsdos
Copy link
Member Author

nielsdos commented Jan 3, 2025

do you expect this will have a positive impact on .phar based CLI applications, e.g. PHPStan in some form?

no

@nielsdos nielsdos marked this pull request as ready for review January 3, 2025 12:58
@nielsdos
Copy link
Member Author

nielsdos commented Jan 3, 2025

@Girgias as promised I made a final pass over the PR to look for lifetime mistakes and I think it's okay.

Girgias
Girgias reviewed Jan 3, 2025
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Okay did a complete pass, only one main comment

ext/phar/util.c Show resolved Hide resolved
ext/phar/zip.c
entry.is_dir = 1;
if(entry.filename_len > 1) {
entry.filename_len--;
if(filename_len > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit which could be done at the same time as editing this line

Suggested change
if(filename_len > 1) {
if (filename_len > 1) {

@nielsdos nielsdos closed this in 21f4211 Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias left review comments

Assignees
No one assigned
Labels
Extension: phar
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nielsdos @Girgias @staabm