[browser][coreCLR] inline dotnetAssert.fastCheck#122871
[browser][coreCLR] inline dotnetAssert.fastCheck#122871pavelsavara merged 3 commits intodotnet:mainfrom
Conversation
pavelsavara
added
arch-wasm
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
There was a problem hiding this comment.
Pull request overview
This PR optimizes performance by inlining calls to dotnetAssert.fastCheck during the build process, eliminating unnecessary string formatting and closure allocation in non-failing cases. A new fastCheck function is introduced that accepts a message factory closure, and a Rollup plugin performs regex-based transformations to inline these calls into direct conditional throws.
Key Changes
- Added a new
regexReplaceRollup plugin to perform regex-based code transformations during the build - Introduced
fastCheckfunction that accepts a closure for lazy message evaluation, with patterns defined to inline it during build - Updated the loader exports table to include
fastCheck, incrementing all subsequent indices appropriately
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/rollup.config.plugins.js | Added new regexReplace plugin using MagicString for code transformations; updated iife2fe to preserve character positions with whitespace |
| src/native/rollup.config.defines.js | Defined regex patterns to match and inline dotnetAssert.fastCheck calls with both backtick and double-quote message factories |
| src/native/rollup.config.js | Integrated the regexReplace plugin with inlinefastCheck patterns into the build pipeline |
| src/native/corehost/browserhost/loader/logging.ts | Introduced new fastCheck function that takes a message factory closure for lazy evaluation; kept existing check function for backward compatibility |
| src/native/libs/Common/JavaScript/types/exchange.ts | Added fastCheck to type definitions for AssertType and LoaderExportsTable |
| src/native/libs/Common/JavaScript/cross-module/index.ts | Updated table indices to account for the new fastCheck entry at position 5, shifting all loader exports forward by one |
| src/native/corehost/browserhost/loader/index.ts | Added fastCheck to exports and populated it in the loader exports table at the correct index |
| src/native/libs/System.Native.Browser/utils/memory.ts | Converted existing dotnetAssert.check calls to dotnetAssert.fastCheck with arrow functions for performance-sensitive validation code |
Co-authored-by: Copilot <[email protected]>
# Conflicts: # src/native/rollup.config.js # src/native/rollup.config.plugins.js
radekdoulik
left a comment
radekdoulik
left a comment
There was a problem hiding this comment.
Thank you! LGTM
As a follow up, it would be great to have a test for that to make sure that the asserts work. Possibly in WBT?
I think you are asking for JS/TS only unit tests, rather than integration tests. I will start thinking about it in #122934 |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| // WASM-TODO: inline the code | ||
| export function check(condition: unknown, message: string): asserts condition { |
There was a problem hiding this comment.
Why do we still need this flavor?
There was a problem hiding this comment.
in places that are not perf critical, that pattern is simpler
inlines calls to
dotnetAssert.fastCheckso that we don't format the string or allocate the closure in non-failing cases.