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

Binary patching and FOMOD Presets #2120

Merged
merged 13 commits into from
Oct 8, 2024
Merged

Binary patching and FOMOD Presets #2120

merged 13 commits into from
Oct 8, 2024

Conversation

halgari
Copy link
Collaborator

@halgari halgari commented Oct 3, 2024

This all ended up being both harder and easier than I expected.

Binary patching - it's implemented and works on the unit tests I included in the code. It's a bit harder than one would expect as often binary patching is paired with replicate that uses a MD5 hash to identify files to install to specific locations. Here's the catch, the MD5 in this case is the MD5 after the patch, and no one ever thought to provide a link between MD5 hashes and patches. And the patch's hash is a CRC32, because why not?

This all means that you kindof have to hash everything in the mod, then apply all patches, then hash any patched files and then you have what you need to install the files. In our case we store the patched files and then link them up by xxHash like any other file.

FOMOD Presets - this ended up being harder than I thought thanks to the utterly broken fomod-installer project. In the end I gave up on trying to pass the options into the installer. Instead we implement a new IGuidedInstaller and emit UserChoice objects based on the presets. The resulting solution is quite clean, just took me a good 4 hours to figure it out.

TL;DR - two of the biggest blockers with collection installs are now solved.

@halgari halgari marked this pull request as ready for review October 3, 2024 03:44
@halgari halgari requested a review from a team October 3, 2024 03:44
@github-actions github-actions bot added the status-needs-rebase Set by CI do not remove label Oct 3, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

This PR conflicts with main. You need to rebase the PR before it can be merged.

# Conflicts:
#	Directory.Packages.props
#	src/Abstractions/NexusMods.Abstractions.Collections/Json/Mod.cs
#	src/NexusMods.Collections/InstallCollectionJob.cs
@github-actions github-actions bot removed the status-needs-rebase Set by CI do not remove label Oct 3, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

This PR doesn't conflict with main anymore. It can be merged after all status checks have passed and it has been reviewed.

@Pickysaurus
Copy link
Contributor

Related #1667

@Sewer56 Sewer56 self-requested a review October 7, 2024 09:39
var step = _steps[_currentStep];

// This looks gross, but it's fairly simple we map through the two trees matching by name, and it's cleaner than 4 nested loops.
var choices =
Copy link
Member

@Sewer56 Sewer56 Oct 7, 2024

Choose a reason for hiding this comment

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

Do we know if the names are case sensitive? I imagine the preset choices aren't set by humans, but if they can be modified by humans, a case insensitive comparison may be better here.


Edit:

Do we also know if the group names are supposed to be unique?
This code assumes that there may be multiple groups with the same name.

i.e. The input does not terminate when installGroup.Name == srcGroup.name, instead, it continues to process all other possible pairs of groups.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to talk with the Vortex devs and figure out these questions, it's not entirely clear to me what the interpretation should be. This works in the cases I tested, but we should find out more

Copy link
Member

@Sewer56 Sewer56 left a comment

Choose a reason for hiding this comment

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

The other feedback is mainly up to whatever you want to do with it; however this piece is blocking.

/// <summary>
/// Completely loads the file into memory
/// </summary>
async Task<Memory<byte>> Load(Hash hash, CancellationToken token = default)
{
await using var stream = await GetFileStream(hash, token);
var memory = new Memory<byte>(GC.AllocateUninitializedArray<byte>((int)stream.Length));
await stream.ReadExactlyAsync(memory, token);
return memory;
}

I have a small number of issues with this:

  • Every chunked size read goes through the entire caching mechanism, which involves a O(N) lookup in LightweightLRUCache for every ChunkSize (usually 1MiB).

    • Because we're always reading forward (ReadToEnd), every read is a cache miss, so we sequentially iterate 16 keys every 1MB read, none of the keys hit, and we rent a new set of bytes from the pool each time.
    • LightweightLRUCache (capacity 16) always rents from the MemoryPool when we add a new cached chunk.
    • Because we have 16 * NumThreads active rentals, number of rentals from global is going to eat a lot of memory, and likely to run out on high core count processors.
    • In fact they will run out. In Shared ArrayPool from runtime, the rentals are per core, (see runtime code), we probably shouldn't use more than 8 at once if using the global pool otherwise every pool rental is a full allocation. (i.e. Reduce LightweightLRUCache capacity to 7, or to 8, and reuse rentals)
    • Because every read is a cache miss, every rental (1MB read) will become a full allocation.
    • tl;dr every 1MB read after 8MB of file is a full allocation; and every read is a cache miss that iterates over 16 keys.
  • This will always run on a single thread, when larger files could be extracted in parallel.

    • If we need control over thread count, that can be a parameter.

I feel very uneasy throwing this specific part to the main branch. At least as part of the IFileStore interface.
Just make an ExtractFile method and strip down the existing code from ExtractFiles to single file; it should end up fairly short in terms of LoC. Thread count is part of Nx' UnpackerSettings


Misc Note:

Bsdiff patches can also technically be bigger than 2GiB in file size, but that point is very moot; most code implementations limit files to 2GiB even if format supports it; and nobody makes patches for files this big because memory usage is astronomical (17 times the input size). Technically with diff algorithms you should be using streams in general; however in this case a full read to RAM is unavoidable since we need to read the file twice to hash it. Without very custom code.


Edit: I also checked GC stuff; no issues there.

@halgari halgari requested a review from Sewer56 October 7, 2024 15:16
@Sewer56
Copy link
Member

Sewer56 commented Oct 7, 2024

Ah, actually, before you merge, you may want to have a way to adjust thread count via parameter. Nx will default to number of physical cores.

Since you're doing the work for bsdiff in parallel already, you may want to limit thread count when calling Load here, so you don't wind up making NumCores^2 threads.

Edit: Either here (Nx via parameter) or on the other end where the patch apply happens.

@Sewer56 Sewer56 merged commit 13f7267 into main Oct 8, 2024
12 checks passed
@erri120 erri120 deleted the collection-install-updates branch October 8, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants