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

Resolve issues 4366 and 4420 for better patch performance #69

Merged
merged 4 commits into from Jun 15, 2014

Conversation

Projects
None yet
3 participants
@heaths
Contributor

heaths commented Jun 14, 2014

I rebased the last two commits here on another change in my PR for branch 'test'. If you take this PR no need for that one. The test changes were necessary but otherwise unrelated, so they were a separate commit.

heaths added some commits Jun 14, 2014

Add disassembly to bundle and package builders
Also handles fact exceptions to avoid an extra break when debugging tests.
Correctly enum all products for non-specific patches
Fixes issue #4366 by correctly parsing the @Validate attribute of patch XML blobs.
Suppress patch sequence data to optimize patch performance
Fixes issue #4420 by allowing for the removal of patch sequence data which is not needed for applicability checks.
Also calls MsiDeterminePatchApplicability instead of MsiDeterminePatchSequence for already-installed patches which is faster for local packages.
@@ -148,6 +150,12 @@ extern "C" HRESULT DAPI WiuInitialize(
vpfnMsiDeterminePatchSequenceW = vpfnMsiDeterminePatchSequenceWFromLibrary;
}
vpfnMsiDetermineApplicablePatchesWFromLibrary = reinterpret_cast<PFN_MSIDETERMINEAPPLICABLEPATCHESW>(::GetProcAddress(vhMsiDll, "MsiDetermineApplicablePatchesW"));
if (NULL == vpfnMsiDetermineApplicablePatchesW)

This comment has been minimized.

@rseanhall

rseanhall Jun 14, 2014

Member

WiX style says to use '!' instead of 'NULL == '.

@rseanhall

rseanhall Jun 14, 2014

Member

WiX style says to use '!' instead of 'NULL == '.

This comment has been minimized.

@heaths

heaths Jun 14, 2014

Contributor

Well aware, but it's consistent with the other code before and after.

@heaths

heaths Jun 14, 2014

Contributor

Well aware, but it's consistent with the other code before and after.

<td>Optional <b>boolean</b> parameter.<br />
<br />
Specifies that the linker should suppress patch sequence data in patch XML to decrease bundle size and increase patch applicability performance (patch packages themselves are not modified).</td>

This comment has been minimized.

@barnson

barnson Jun 15, 2014

Member

There's no documented downside. Is there any downside? (i.e., this is just opt-in to new, better but changed behavior?)

@barnson

barnson Jun 15, 2014

Member

There's no documented downside. Is there any downside? (i.e., this is just opt-in to new, better but changed behavior?)

This comment has been minimized.

@heaths

heaths Jun 15, 2014

Contributor

Pretty much, yeah. When I made the change for VSU certain people were nervous that it was necessary despite plenty of evidence to the contrary, so I made it opt-in.

@heaths

heaths Jun 15, 2014

Contributor

Pretty much, yeah. When I made the change for VSU certain people were nervous that it was necessary despite plenty of evidence to the contrary, so I made it opt-in.

This comment has been minimized.

@barnson

barnson Jun 15, 2014

Member

Cool. We should look at making it the default in v4.0.

@barnson

barnson Jun 15, 2014

Member

Cool. We should look at making it the default in v4.0.

if (this.additionalProperties.ContainsKey(name))
{
this.additionalProperties[name] = value;

This comment has been minimized.

@barnson

barnson Jun 15, 2014

Member

Why not use the indexer all the time and skip the ContainsKey check? Probably boils down to similar code in the BCL...

@barnson

barnson Jun 15, 2014

Member

Why not use the indexer all the time and skip the ContainsKey check? Probably boils down to similar code in the BCL...

This comment has been minimized.

@rseanhall

rseanhall Jun 15, 2014

Member

Because it will throw a KeyNotFoundException.

Edit: never mind, I guess it doesn't do that when setting it.

@rseanhall

rseanhall Jun 15, 2014

Member

Because it will throw a KeyNotFoundException.

Edit: never mind, I guess it doesn't do that when setting it.

This comment has been minimized.

@barnson

barnson Jun 15, 2014

Member

Not when adding a key/value.

@barnson

barnson Jun 15, 2014

Member

Not when adding a key/value.

This comment has been minimized.

@heaths

heaths Jun 15, 2014

Contributor

Perhaps an older version did, because that was the experience sometime back. Or maybe it was when I used a different base class. We're talking O(1) here, so I'm not that concerned unless you want me to submit a post-merge commit.

@heaths

heaths Jun 15, 2014

Contributor

Perhaps an older version did, because that was the experience sometime back. Or maybe it was when I used a different base class. We're talking O(1) here, so I'm not that concerned unless you want me to submit a post-merge commit.

This comment has been minimized.

@barnson

barnson Jun 15, 2014

Member

Just a simplification. Low pri.

@barnson

barnson Jun 15, 2014

Member

Just a simplification. Low pri.

This comment has been minimized.

@heaths

heaths Jun 15, 2014

Contributor

No problem. I'm running some tests now to make sure other things work (now that all the results are combined - despite using Git, internally changes are few enough that code flow is basically linear) and will submit a post-merge fix up PR.

@heaths

heaths Jun 15, 2014

Contributor

No problem. I'm running some tests now to make sure other things work (now that all the results are combined - despite using Git, internally changes are few enough that code flow is basically linear) and will submit a post-merge fix up PR.

barnson added a commit that referenced this pull request Jun 15, 2014

Merge pull request #69 from heaths/patchperf
Resolve issues 4366 and 4420 for better patch performance

@barnson barnson merged commit af82204 into wixtoolset:develop Jun 15, 2014

@heaths heaths deleted the heaths:patchperf branch Jun 15, 2014

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