Skip to content

[browser] Move reflection from C# to C - #98534

Merged
pavelsavara merged 2 commits into
dotnet:mainfrom
pavelsavara:browser_reflection_c
Feb 16, 2024
Merged

[browser] Move reflection from C# to C#98534
pavelsavara merged 2 commits into
dotnet:mainfrom
pavelsavara:browser_reflection_c

Conversation

@pavelsavara

@pavelsavarapavelsavara commented Feb 16, 2024

Copy link
Copy Markdown
Member

To address size+perf regression #98391 (comment)

@pavelsavarapavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Feb 16, 2024
@pavelsavarapavelsavara added this to the 9.0.0 milestone Feb 16, 2024
@pavelsavarapavelsavara self-assigned this Feb 16, 2024
@ghost

ghost commented Feb 16, 2024

Copy link
Copy Markdown

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

To address size+perf regression #98391 (comment)

Author:pavelsavara
Assignees:pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript, os-browser

Milestone:9.0.0

This was referenced Feb 16, 2024
Open
Closed
Closed
Closed
Closed
@pavelsavara

pavelsavara commented Feb 16, 2024

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-wasm

@pavelsavara
pavelsavara marked this pull request as ready for review February 16, 2024 11:48
@azure-pipelines

azure-pipelinesBot commented Feb 16, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

ilonatommy
ilonatommy approved these changes Feb 16, 2024
View reviewed changes
Comment threadsrc/mono/browser/runtime/marshal-to-js.ts
maraf
maraf reviewed Feb 16, 2024
View reviewed changes
Comment threadsrc/mono/browser/runtime/corebindings.c
Comment thread...ropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs
{
throw new ArgumentNullException(nameof(fullyQualifiedName));
}
var (assemblyName, nameSpace, shortClassName, methodName) = ParseFQN(fullyQualifiedName);

@marafmarafFeb 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we don't need parsed values in C#. Would it make any difference to parse it in C? Or in JS?

@pavelsavarapavelsavaraFeb 16, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

JS is in different thread, needs to pass via JS interop as C# String
C is in current/managed thread and needs to be UTF8.
I guess this is OK.

To really optimize it we should get rid of reflection and use [UnmanagedCalersOnly] from roslyn code gen. That will require API change tho.

@pavelsavara
pavelsavara merged commit de12fe7 into dotnet:mainFeb 16, 2024
@pavelsavara
pavelsavara deleted the browser_reflection_c branch February 16, 2024 14:05
@pavelsavarapavelsavara mentioned this pull request Feb 16, 2024
Merged
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 18, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lewinglewinglewing left review comments
@marafmarafmaraf approved these changes
@ilonatommyilonatommyilonatommy approved these changes
@radekdoulikradekdoulikAwaiting requested review from radekdoulik
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

Assignees

@pavelsavarapavelsavara

Labels

arch-wasmWebAssembly architecturearea-System.Runtime.InteropServices.JavaScriptos-browserBrowser variant of arch-wasm

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@pavelsavara@lewing@maraf@ilonatommy