Skip to content

Commit

Permalink
phar: use crc32 bulk method instead. (#6099)
Browse files Browse the repository at this point in the history 
phar: use crc32 bulk method instead.

Benefit from the hardware crc32 computing.

Signed-off-by: Frank Du <frank.du@intel.com>
  • Loading branch information
@frankdjx
frankdjx authored Jun 19, 2021
1 parent 578b67d commit 28a1a6b
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 28 deletions.
15 changes: 5 additions & 10 deletions ext/phar/phar.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2374,7 +2374,7 @@ int phar_open_executed_filename(char *alias, size_t alias_len, char **error) /*
int phar_postprocess_file(phar_entry_data *idata, uint32_t crc32, char **error, int process_zip) /* {{{ */
{
uint32_t crc = ~0;
int len = idata->internal_file->uncompressed_filesize;
int len = idata->internal_file->uncompressed_filesize, ret;
php_stream *fp = idata->fp;
phar_entry_info *entry = idata->internal_file;

Expand Down Expand Up @@ -2439,13 +2439,11 @@ int phar_postprocess_file(phar_entry_data *idata, uint32_t crc32, char **error,

php_stream_seek(fp, idata->zero, SEEK_SET);

while (len--) {
CRC32(crc, php_stream_getc(fp));
}
ret = crc32_stream_bulk_update(&crc, fp, len);

php_stream_seek(fp, idata->zero, SEEK_SET);

if (~crc == crc32) {
if (SUCCESS == ret && ~crc == crc32) {
entry->is_crc_checked = 1;
return SUCCESS;
} else {
Expand Down Expand Up @@ -2539,7 +2537,7 @@ int phar_flush(phar_archive_data *phar, char *user_stub, zend_long len, int conv
zend_off_t manifest_ftell;
zend_long offset;
size_t wrote;
uint32_t manifest_len, mytime, loc, new_manifest_count;
uint32_t manifest_len, mytime, new_manifest_count;
uint32_t newcrc32;
php_stream *file, *oldfile, *newfile, *stubfile;
php_stream_filter *filter;
Expand Down Expand Up @@ -2796,10 +2794,7 @@ int phar_flush(phar_archive_data *phar, char *user_stub, zend_long len, int conv
return EOF;
}
newcrc32 = ~0;
mytime = entry->uncompressed_filesize;
for (loc = 0;loc < mytime; ++loc) {
CRC32(newcrc32, php_stream_getc(file));
}
crc32_stream_bulk_update(&newcrc32, file, entry->uncompressed_filesize);
entry->crc32 = ~newcrc32;
entry->is_crc_checked = 1;
if (!(entry->flags & PHAR_ENT_COMPRESSION_MASK)) {
Expand Down
5 changes: 1 addition & 4 deletions ext/phar/zip.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,6 @@ static int phar_zip_changed_apply_int(phar_entry_info *entry, void *arg) /* {{{

/* do extra field for perms later */
if (entry->is_modified) {
uint32_t loc;
php_stream_filter *filter;
php_stream *efp;

Expand Down Expand Up @@ -915,9 +914,7 @@ static int phar_zip_changed_apply_int(phar_entry_info *entry, void *arg) /* {{{
efp = phar_get_efp(entry, 0);
newcrc32 = ~0;

for (loc = 0;loc < entry->uncompressed_filesize; ++loc) {
CRC32(newcrc32, php_stream_getc(efp));
}
crc32_stream_bulk_update(&newcrc32, efp, entry->uncompressed_filesize);

entry->crc32 = ~newcrc32;
PHAR_SET_32(central.uncompsize, entry->uncompressed_filesize);
Expand Down
58 changes: 44 additions & 14 deletions ext/standard/crc32.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,24 +89,12 @@ static uint32_t crc32_aarch64(uint32_t crc, char *p, size_t nr) {
# endif
#endif

/* {{{ Calculate the crc32 polynomial of a string */
PHP_FUNCTION(crc32)
uint32_t crc32_bulk_update(uint32_t crc, const char *p, size_t nr)
{
char *p;
size_t nr;
uint32_t crcinit = 0;
uint32_t crc;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STRING(p, nr)
ZEND_PARSE_PARAMETERS_END();

crc = crcinit^0xFFFFFFFF;

#if HAVE_AARCH64_CRC32
if (has_crc32_insn()) {
crc = crc32_aarch64(crc, p, nr);
RETURN_LONG(crc^0xFFFFFFFF);
return crc;
}
#endif

Expand All @@ -116,9 +104,51 @@ PHP_FUNCTION(crc32)
p += nr_simd;
#endif

/* The trailing part */
for (; nr--; ++p) {
crc = ((crc >> 8) & 0x00FFFFFF) ^ crc32tab[(crc ^ (*p)) & 0xFF ];
}

return crc;
}

int crc32_stream_bulk_update(uint32_t *crc, php_stream *fp, size_t nr)
{
size_t handled = 0, n;
char buf[1024];

while (handled < nr) {
n = nr - handled;
n = (n < sizeof(buf)) ? n : sizeof(buf); /* tweak to buf size */

n = php_stream_read(fp, buf, n);
if (n > 0) {
*crc = crc32_bulk_update(*crc, buf, n);
handled += n;
} else { /* EOF */
return FAILURE;
}
}

return SUCCESS;
}

/* {{{ Calculate the crc32 polynomial of a string */
PHP_FUNCTION(crc32)
{
char *p;
size_t nr;
uint32_t crcinit = 0;
uint32_t crc;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STRING(p, nr)
ZEND_PARSE_PARAMETERS_END();

crc = crcinit^0xFFFFFFFF;

crc = crc32_bulk_update(crc, p, nr);

RETURN_LONG(crc^0xFFFFFFFF);

This comment has been minimized.

Sign in to view

Copy link
@remicollet

remicollet Jun 22, 2021 •
edited
Loading

Member

To be cleaner, especially as now public and used in other extension (at least phar), I think we need init / end function (macro ?)

crc = crc32_bulk_init();
crc = crc32_bulk_update(crc, p, nr);
crc = crc32_bulk_end(crc).

This comment has been minimized.

Sign in to view

Copy link
@weltling

weltling Jun 24, 2021

Contributor

Good catch. Would sure make sense for the consistency. It probably should be also properly exported through PHPAPI, right now it isn't which might lead to visibility issues.

Thanks

This comment has been minimized.

Sign in to view

Copy link
@remicollet

remicollet Jun 24, 2021

Member

BTW, may be simpler to drop this new implementation and use the one in ext/hash

This comment has been minimized.

Sign in to view

Copy link
@remicollet

remicollet Jun 24, 2021 •
edited
Loading

Member

@weltling and FYI missing PHPAPI is a major blocker for phar when built shared
b83bfb2

This comment has been minimized.

Sign in to view

Copy link
@weltling

weltling Jun 26, 2021

Contributor

@remicollet thanks for the fix. AFAIR phar were in most case statically linked, but well it's the consumer choice.

Thanks

This comment has been minimized.

Sign in to view

Copy link
@weltling

weltling Jul 3, 2021

Contributor

@remicollet i've added the suggested API here e7123ef. Regarding replacing the implementation with hash - seems there's a bit more and both implementation need to be merged before, see the piece for the ARM intrinsic support. Lets see, perhaps a better plan is the next minor. Having just one implementation everywhere would be indeed best.

Thanks
}
/* }}} */
5 changes: 5 additions & 0 deletions ext/standard/crc32.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@

#define CRC32(crc, ch) (crc = (crc >> 8) ^ crc32tab[(crc ^ (ch)) & 0xff])

uint32_t crc32_bulk_update(uint32_t crc, const char *p, size_t nr);

/* Return FAILURE if stream reading fail */
int crc32_stream_bulk_update(uint32_t *crc, php_stream *fp, size_t nr);

/* generated using the AUTODIN II polynomial
* x^32 + x^26 + x^23 + x^22 + x^16 +
* x^12 + x^11 + x^10 + x^8 + x^7 + x^5 + x^4 + x^2 + x^1 + 1
Expand Down

0 comments on commit 28a1a6b

Please sign in to comment.