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

Improve virion_infect performance #239

Open
wants to merge 5 commits into
base: beta
Choose a base branch
from

Conversation

Muqsit
Copy link

@Muqsit Muqsit commented Aug 26, 2020

Significantly improves performance of virion_infect by placing changed files in a temporary directory and calling Phar::buildFromDirectory($temp_dir_path). This avoids repackaging phar for every changed file (repackaging happens only during the Phar::buildFromDirectory call).

The complexity is reduced from O(n^2) all the way to O(n) where n is the number of files being packaged into .phar.

assets/php/virion.php Outdated Show resolved Hide resolved
unlink($this->path);
$this->path .= DIRECTORY_SEPARATOR;
mkdir($this->path, 0666);
}
Copy link
Member

Choose a reason for hiding this comment

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

Please make this error-safe. Consider race conditions of path being used by another process between unlink() and mkdir().

Copy link
Member

Choose a reason for hiding this comment

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

Now you have the chance of two virions building in the same directory and mixing up.

Copy link
Author

Choose a reason for hiding this comment

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

I can't think of a way to do this without either throwing an exception (if(!mkdir()) throw $e) or a possibly long-lasting while(!mkdir()) sleep() loop. I thought of going with if(!is_dir()) mkdir() because the only other mkdir call in poggit/poggit goes with that approach, any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Choose another tempnam if the directory creation failed. Only run the subsequent code if you are the one who created this directory.

Copy link
Member

Choose a reason for hiding this comment

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

@Muqsit you had a chance to do this ?

Copy link
Member

Choose a reason for hiding this comment

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

I am not confident that the logic is still correct... If $this->path doesn't already exist, something might have gone wrong, and you should choose another tempnam. If $this->path is already a directory, this also means something is wrong, and you should also choose another tempnam.

Copy link
Author

Choose a reason for hiding this comment

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

While I do understand the issue, I unfortunately don't have much knowledge in this field and would prefer someone else take over this task as deleting the wrong directory could lead to unperceivable results, possibly even out of the realms of this program.

Copy link
Member

Choose a reason for hiding this comment

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

what about sticking getmypid() on the end of the folder name?

assets/php/virion.php Outdated Show resolved Hide resolved
unlink($this->path);
$this->path .= DIRECTORY_SEPARATOR;
mkdir($this->path, 0666);
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not confident that the logic is still correct... If $this->path doesn't already exist, something might have gone wrong, and you should choose another tempnam. If $this->path is already a directory, this also means something is wrong, and you should also choose another tempnam.

@SOF3
Copy link
Member

SOF3 commented Jan 19, 2021

I think the temp dir thing is rare enough to matter. @JaxkDev any other comments?

@JaxkDev
Copy link
Member

JaxkDev commented Jan 19, 2021

I think the temp dir thing is rare enough to matter. @JaxkDev any other comments?

I dont touch virions lol

@SOF3
Copy link
Member

SOF3 commented Jan 19, 2021

The complexity is reduced from O(n!) all the way to O(n) where n is the number of files being packaged into .phar.

Are you sure it is O(n!)? It just sounds like O(n^2) to me.

@Muqsit
Copy link
Author

Muqsit commented Jan 19, 2021

The complexity is reduced from O(n!) all the way to O(n) where n is the number of files being packaged into .phar.

Are you sure it is O(n!)? It just sounds like O(n^2) to me.

Yeah. Previously, for each file that was inserted in Phar, n+1 files would be repackaged (n = number of files before the insertion of the new file). I would guess that makes it n!?

@SOF3
Copy link
Member

SOF3 commented Jan 19, 2021

The complexity is reduced from O(n!) all the way to O(n) where n is the number of files being packaged into .phar.

Are you sure it is O(n!)? It just sounds like O(n^2) to me.

Yeah. Previously, for each file that was inserted in Phar, n+1 files would be repackaged (n = number of files before the insertion of the new file). I would guess that makes it n!?

No, that's O(n^2). To be precise, it is. 1 + 2 + ... + n = n(n+1)/2.

O(n!) multiplies them together instead of adding. That would take centuries for something like PocketMine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants