Migrate to zlib-ng, part 2: consume it in runtime - #102403

Merged
carlossanlop merged 39 commits into
dotnet:mainfrom
carlossanlop:zlib-ng
Jul 3, 2024
Merged

Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom
carlossanlop:zlib-ng

Conversation

@carlossanlop

@carlossanlopcarlossanlop commented May 17, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Contributes to: #101465

  • Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  • Updated cmake code
  • Confirmed full local build on Linux passed
  • Confirmed full local build on Windows passed (including NativeAOT)
  • Confirmed full local build on MacOS passed
  • Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  • Fix MacOS issues in CI
  • Fix some quick temporary workarounds
  • Compare performance before and after on Linux and Windows, on x64 and arm64
  • If zlib-ng is found installed in the current distro, use that instead.
  • Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  • Run runtime-community and fix any issues
  • Run runtime-nativeaot-outerloop and fix any issues
  • Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.

Performance comparisons

dotnet/performance benchmark results: #102403 (comment)

Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)

@carlossanlopcarlossanlop self-assigned this May 17, 2024
jkoritzinsky
jkoritzinsky reviewed May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake Outdated
@jkotas

jkotas commented May 17, 2024

Copy link
Copy Markdown
Member

To make this easier to review, it would be best to split this into 3 PRs:

  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references

@carlossanlop

carlossanlop commented May 17, 2024

Copy link
Copy Markdown
ContributorAuthor

@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.

stephentoub
stephentoub reviewed May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop

carlossanlop commented May 20, 2024

Copy link
Copy Markdown
ContributorAuthor

To make this easier to review, it would be best to split this into 3 PRs:

@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0

@jkotas

jkotas commented May 21, 2024

Copy link
Copy Markdown
Member

The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.

Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.

@jkotas

jkotas commented May 21, 2024

Copy link
Copy Markdown
Member

To make this easier to review, it would be best to split this into 3 PRs:

I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0

This is better than nothing, but it is still pretty clunky experience.

FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.

jkotas
jkotas reviewed May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake Outdated
@build-analysisbuild-analysisBot mentioned this pull request May 21, 2024
Closed
@carlossanlopcarlossanlop mentioned this pull request May 21, 2024
Merged
@carlossanlop

carlossanlop commented May 21, 2024

Copy link
Copy Markdown
ContributorAuthor

As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.

I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).

@carlossanlopcarlossanlop changed the title Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop

carlossanlop commented May 22, 2024

Copy link
Copy Markdown
ContributorAuthor

Big difference now: 23 files instead of 388.

carlossanlop
carlossanlop commented May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt Outdated
Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt Outdated
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt Outdated
Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c Outdated
jkoritzinsky
jkoritzinsky reviewed May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake Outdated
Comment threadsrc/native/external/zlib-ng.cmake Outdated
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt Outdated
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c Outdated
Comment threadeng/native/configurecompiler.cmake Outdated
@carlossanlop

carlossanlop commented May 23, 2024

Copy link
Copy Markdown
ContributorAuthor

I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.

akoeplinger
akoeplinger reviewed May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt Outdated
Comment threadsrc/mono/mono/mini/CMakeLists.txt Outdated
Comment threadsrc/mono/mono/mini/CMakeLists.txt Outdated
Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c Outdated
@akoeplinger

akoeplinger commented May 23, 2024

Copy link
Copy Markdown
Member

The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?

I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)

@carlossanlop

carlossanlop commented May 23, 2024

Copy link
Copy Markdown
ContributorAuthor

I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)

The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?

@build-analysisbuild-analysisBot mentioned this pull request May 24, 2024
Open
lambdageek
lambdageek reviewed May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt Outdated
@build-analysisbuild-analysisBot mentioned this pull request May 31, 2024
Closed
@akoeplinger

akoeplinger commented May 31, 2024

Copy link
Copy Markdown
Member

Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?

No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.

@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop commented May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt Outdated
@mtl1979

mtl1979 commented Jul 2, 2024

Copy link
Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.

@carlossanlop

carlossanlop commented Jul 3, 2024

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-community

@azure-pipelines

This comment was marked as outdated.

Sign in to view
This was referenced Jul 3, 2024
Open
Closed
@carlossanlop

carlossanlop commented Jul 3, 2024

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-nativeaot-outerloop

@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop

carlossanlop commented Jul 3, 2024

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-extra-platforms

@azure-pipelines

This comment was marked as outdated.

Sign in to view
This was referenced Jul 3, 2024
Closed
Closed
This was referenced Jul 3, 2024
Closed
Closed
@carlossanlopcarlossanlop mentioned this pull request Jul 3, 2024
Closed
@carlossanlop

carlossanlop commented Jul 3, 2024

Copy link
Copy Markdown
ContributorAuthor

/ba-g all failures have been investigated, none are related to this PR

@carlossanlop
carlossanlop merged commit f5c9a5e into dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop deleted the zlib-ng branch July 3, 2024 06:26
rhuijben
rhuijben reviewed Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h
This was referenced Jul 3, 2024
Closed
Merged
agocke added a commit that referenced this pull request Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke mentioned this pull request Jul 4, 2024
Merged
ilonatommy pushed a commit that referenced this pull request Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop

carlossanlop commented Jul 4, 2024

Copy link
Copy Markdown
ContributorAuthor

This was reverted because it broke the official build. I'll take a look soon.

@carlossanlop
carlossanlop restored the zlib-ng branch July 4, 2024 17:29
@carlossanlopcarlossanlop mentioned this pull request Jul 4, 2024
Merged
@carlossanlop
carlossanlop deleted the zlib-ng branch July 5, 2024 20:44
carlossanlop added a commit that referenced this pull request Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop mentioned this pull request Jul 16, 2024
Open
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub
+2 more reviewers
@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Migrate to zlib-ng, part 2: consume it in runtime - #102403
Merged
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Jul 3, 2024
Merged
Migrate to zlib-ng, part 2: consume it in runtime#102403
carlossanlop merged 39 commits into
dotnet:mainfrom 
carlossanlop:zlib-ng
Conversation
@carlossanlop
@carlossanlopcarlossanlop
commented
May 17, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Contributes to: #101465
  •  Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  •  Updated cmake code
  •  Confirmed full local build on Linux passed
  •  Confirmed full local build on Windows passed (including NativeAOT)
  •  Confirmed full local build on MacOS passed
  •  Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  •  Fix MacOS issues in CI
  •  Fix some quick temporary workarounds
  •  Compare performance before and after on Linux and Windows, on x64 and arm64
  •  If zlib-ng is found installed in the current distro, use that instead.
  •  Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  •  Run runtime-community and fix any issues
  •  Run runtime-nativeaot-outerloop and fix any issues
  •  Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.
Performance comparisons
dotnet/performance benchmark results: #102403 (comment)
Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)
@carlossanlopcarlossanlop
self-assigned this
May 17, 2024
jkoritzinsky
jkoritzinsky
reviewed
May 17, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-ng.cmake
Outdated

@jkotas
jkotas
commented
May 17, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references
@carlossanlop
carlossanlop
commented
May 17, 2024

Copy link


Copy Markdown

ContributorAuthor
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.
stephentoub
stephentoub
reviewed
May 20, 2024
View reviewed changes
Comment threadTHIRD-PARTY-NOTICES.TXT
@carlossanlop
carlossanlop
commented
May 20, 2024

Copy link


Copy Markdown

ContributorAuthor
To make this easier to review, it would be best to split this into 3 PRs:
@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build.
Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo.
@jkotas
jkotas
commented
May 21, 2024

Copy link


Copy Markdown

Member
To make this easier to review, it would be best to split this into 3 PRs:
I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0
This is better than nothing, but it is still pretty clunky experience.
FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster.
jkotas
jkotas
reviewed
May 21, 2024
View reviewed changes
Comment threadeng/native/configurecompiler.cmake
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 21, 2024

Closed

@carlossanlopcarlossanlop
mentioned this pull request
May 21, 2024
 Merged

@carlossanlop
carlossanlop
commented
May 21, 2024

Copy link


Copy Markdown

ContributorAuthor
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet.
I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR).
@carlossanlopcarlossanlop
changed the title
Migrate to zlib-ngMigrate to zlib-ng, part 2: consume it in runtimeMay 21, 2024
@carlossanlop
carlossanlop
commented
May 22, 2024

Copy link


Copy Markdown

ContributorAuthor
Big difference now: 23 files instead of 388.
carlossanlop
carlossanlop
commented
May 22, 2024
View reviewed changes
Comment threadsrc/mono/CMakeLists.txt
Comment threadsrc/mono/mono/metadata/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/metadata/debug-mono-ppdb.c
Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/external/zlib-ng/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.h
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

jkoritzinsky
jkoritzinsky
reviewed
May 22, 2024
View reviewed changes
Comment threadsrc/native/external/zlib-intel.cmake
Outdated

Comment threadsrc/native/external/zlib-ng.cmake
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Comment threadsrc/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated

Comment threadeng/native/configurecompiler.cmake
Outdated

@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.
akoeplinger
akoeplinger
reviewed
May 23, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

Comment threadsrc/native/libs/System.IO.Compression.Native/pal_zlib.c
Outdated

@akoeplinger
akoeplinger
commented
May 23, 2024

Copy link


Copy Markdown

Member
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers?
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
@carlossanlop
carlossanlop
commented
May 23, 2024

Copy link


Copy Markdown

ContributorAuthor
I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Open

lambdageek
lambdageek
reviewed
May 29, 2024
View reviewed changes
Comment threadsrc/mono/mono/mini/CMakeLists.txt
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
May 31, 2024

Closed

@akoeplinger
akoeplinger
commented
May 31, 2024

Copy link


Copy Markdown

Member
Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen?
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict.
@carlossanlop

This comment was marked as resolved.

Sign in to view
carlossanlop
carlossanlop
commented
May 31, 2024
View reviewed changes
Comment threadsrc/native/libs/System.IO.Compression.Native/CMakeLists.txt
Outdated

@mtl1979
mtl1979
commented
Jul 2, 2024

Copy link


Copy Markdown

ARMv6 support in zlib-ng is pretty new and still incomplete... We used to require minimum of ARMv8 as older processors are pretty rare nowadays and don't really benefit much (or any) from any of the currently implemented vector optimizations.
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-community
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Open


Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-nativeaot-outerloop
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-extra-platforms
@azure-pipelines

This comment was marked as outdated.

Sign in to view

This was referenced Jul 3, 2024

Closed


Closed


This was referenced Jul 3, 2024

Closed


Closed

@carlossanlopcarlossanlop
mentioned this pull request
Jul 3, 2024

Closed

@carlossanlop
carlossanlop
commented
Jul 3, 2024

Copy link


Copy Markdown

ContributorAuthor
/ba-g all failures have been investigated, none are related to this PR
@carlossanlop
carlossanlop
merged commit f5c9a5e
into
dotnet:mainJul 3, 2024
@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 3, 2024 06:26
rhuijben
rhuijben
reviewed
Jul 3, 2024
View reviewed changes
Comment threadsrc/native/libs/Common/pal_utilities.h

This was referenced Jul 3, 2024

Closed

 Merged

agocke added a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)"
9c3cb28
This reverts commit f5c9a5e.
@agockeagocke
mentioned this pull request
Jul 4, 2024
 Merged

ilonatommy pushed a commit
that referenced
this pull request
Jul 4, 2024
@agocke
Revert "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#…
5b86dca
…104414)
This reverts commit f5c9a5e.
@carlossanlop
carlossanlop
commented
Jul 4, 2024

Copy link


Copy Markdown

ContributorAuthor
This was reverted because it broke the official build. I'll take a look soon.
@carlossanlop
carlossanlop
restored the

zlib-ng

branch
July 4, 2024 17:29
@carlossanlopcarlossanlop
mentioned this pull request
Jul 4, 2024
 Merged

@carlossanlop
carlossanlop
deleted the

zlib-ng

branch
July 5, 2024 20:44
carlossanlop added a commit
that referenced
this pull request
Jul 8, 2024
@carlossanlop
Migrate to zlib-ng, part 2: consume it in runtime (second attempt) (#…
cf08d43
…104454)
* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:
build.cmd -ci -arch x64 -os windows -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@carlossanlopcarlossanlop
mentioned this pull request
Jul 16, 2024

Open

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Aug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@lewinglewingAwaiting requested review from lewinglewing is a code owner
@pavelsavarapavelsavaraAwaiting requested review from pavelsavarapavelsavara is a code owner
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@lateralusXlateralusXAwaiting requested review from lateralusXlateralusX is a code owner
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@lambdageeklambdageekAwaiting requested review from lambdageek
@akoeplingerakoeplingerAwaiting requested review from akoeplinger
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@ViktorHoferViktorHoferAwaiting requested review from ViktorHofer
@ericstjericstjAwaiting requested review from ericstj
@stephentoubstephentoubAwaiting requested review from stephentoub

+2 more reviewers

@rhuijbenrhuijbenrhuijben left review comments
@directhexdirecthexdirecthex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlopcarlossanlop

Labels

area-System.IO.Compression

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

14 participants

@carlossanlop@jkotas@akoeplinger@nmoinvaz@mtl1979@Dead2@directhex@steveisok@lambdageek@jkoritzinsky@rhuijben@stephentoub@ViktorHofer@ericstj