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

ISO extraction: do allocation at file creation time #1445

Closed
wants to merge 1 commit into from

Conversation

Mattiwatti
Copy link
Contributor

Some time ago a PR of mine was merged in which I added preallocate_filesize() to improve ISO extraction performance. However I overlooked a fairly simple way to achieve the same thing in a simpler and faster way: the native NtCreateFile API accepts an AllocationSize parameter which will accomplish the same thing. This saves two system calls per file, plus the trips to the filesystem that come with modifying FileEndOfFileInfo/FileAllocationInfo.

I'm aware that winternl.h tells users to use CreateFile instead, but my arguments against this are:

  1. CreateFile does not allow specifying an allocation size.
  2. NtCreateFile is in fact extensively documented (perhaps better than CreateFile) as ZwCreateFile on MSDN.
  3. winternl.h is a worthless header that purposely defines incomplete types because they are 'undocumented', which is worse than not defining them. Therefore I don't hold the opinion of whoever wrote the comment regarding CreateFile in very high regard.

CreatePreallocatedFileU aims to be a fairly complete replacement for CreateFile (within reason) so that it can potentially be used in other places later, although ISO extraction is probably the place where it provides the most benefit. This means it supports all flags and attributes CreateFile does, with the exception of one: the hTemplate parameter was removed because it is not used in Rufus, and furthermore it requires querying EAs from the filesystem, which (1) takes up an obscene amount of code, and (2) is not supported by all filesystems anyway. The code was adapted from ReactOS' CreateFileW. Both ReactOS and Rufus are licensed under the GPL so I don't believe this should be a problem.

In a few rough tests I did this improves performance by 5-35% with FAT32, and 0-5% with NTFS. For FAT32, ISOs with many files (e.g. Windows XP) benefit the most, while those with most of their data in a single file (e.g. Ubuntu) only about 5%. Extracting a Windows 10 ISO is about 15% faster with FAT32.

* Implement CreatePreallocatedFileU which uses NtCreateFile to create files with preallocated sizes. This is used during ISO extraction to improve performance
* Minor changes to process.c/.h to prevent macro/typedef conflicts
* Remove now-unused preallocate_filesize which was called after CreateFileU
@pbatard
Copy link
Owner

pbatard commented Feb 4, 2020

Code looks good to me at first glance (though it's of course a bit nicer when PR use the same code formatting style as the original code, such as { positioning and stuff, but I'll fix that), and I'm always happy to apply changes that can improve speed.

I'll check and test this a bit more closely when I get a chance, which may not be for another few days (or weeks, depending) and then apply it. Many thanks for the contribution.

@Mattiwatti
Copy link
Contributor Author

Sorry about that; I normally make an effort to stick to whatever formatting is already used in a project, but I overlooked the brace style on if-statements and such. I can push a fix commit to update it, but to be honest you would probably do a better job of it, seeing as code style is mostly a personal thing.

@pbatard
Copy link
Owner

pbatard commented Feb 4, 2020

Yeah, no worry, the patch is not that long for me to modify and I may tweak a few other things anyway.

@pbatard pbatard added this to the 3.9 milestone Feb 13, 2020
@pbatard
Copy link
Owner

pbatard commented Feb 14, 2020

I have now applied your patch. Apart from the formatting (which got taken care of automatically from copy/pasting in VS2019) the only big difference is that I moved CreatePreallocatedFile() to stdio.c which is more suitable than msapi_utf8.h since we're not actually converting an already existing CreatePreallocatedFileW() to UTF-8 and also since this is the place where we have other custom I/O functions, such as WriteFileWithRetry().

Everything looks good to me, and I think I see a noticeable improvement in extraction speed as well, though I haven't really timed it. So thanks again for submitting this PR!

@pbatard pbatard closed this in 4c5adf0 Feb 14, 2020
@Mattiwatti Mattiwatti deleted the ntcreatefile branch February 20, 2020 11:08
dyeske pushed a commit to dyeske/rufus that referenced this pull request Mar 13, 2020
* Implement CreatePreallocatedFile() which uses NtCreateFile() to create files with preallocated sizes.
  This is used during ISO extraction to improve performance.
* Remove now-unused preallocate_filesize which was called after CreateFileU().
* Closes pbatard#1445
@lock
Copy link

lock bot commented May 20, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@lock lock bot locked and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants