Improvements for delegate types #516

rynowak · 2018-04-09T01:56:44Z

See commits for details

rynowak · 2018-04-09T02:01:19Z

src/Microsoft.AspNetCore.Blazor/Components/BindMethods.cs

            where T : UIEventArgs
        {
-            return e => value((T)e);
+            return value;


This is the payoff. This will now 'just work', and doesn't cause a diff each time the component renders (except in the case of a capturing lambda).

The advantage of having the tag helper at all (vs no language support) is that because we know what T is at compile time, is that you get a much better experience using a lambda. Without the language support, lambdas would have to be UIEventHandler only

rynowak · 2018-04-09T02:02:22Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeBuilder.cs

+        /// <see cref="UIEventHandlerRenderTreeBuilderExtensions.AddAttribute(RenderTreeBuilder, int, string, UIChangeEventHandler)"/>
+        /// that calls this method.
+        /// </remarks>
+        public void AddAttribute(int sequence, string name, MulticastDelegate value)


See the commit message for details. Tuning the overload set of of AddAttribute to make it do everything we want was a little tricky.

rynowak · 2018-04-09T02:04:24Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffBuilder.cs

+                    typeof(UIEventArgs).IsAssignableFrom(parameters[0].ParameterType))
+                {
+                    diffContext.Renderer.AssignEventHandlerId(ref newFrame);
+                }


This is the one part I wasn't totally happy with, I'm interested to know if you have better ideas. I'd really like to avoid running quite so much code during the render phase.

data points:

I assume we only want to 'register' delegates that can receive dom events

therefore it's useful to know if a delegate handles a dom event based on its type

therefore keeping UIEventArgs allows us to know what delegates do and do not respond to dom events.

I assume we only want to 'register' delegates that can receive dom events

I'm not sure that's so important. Are there many likely scenarios where someone tries to use onsomething=@myDelegate where myDelegate isn't a UIEventHandler? Even if someone does that and we assign an event handler ID that never gets used, that doesn't seem like a big problem.

I'd favor optimizing for the common case (where the delegate is a UIEventHandler) even if in the uncommon case we do something a little less inefficient.

Are you suggesting we just register all delegates then?

At this layer we're just looking at the attributes values, and this gets called for all delegate values. Without this code don't know if something was intended to be event-like or not.

We could end up doing something like "if it's on a component, leave it alone; if it's on an HTML element, then assign IDs to all delegate values" since there's no use case for delegate-typed HTML attribute values other than event handlers. I know we don't have the "component or not" info directly available here but could find some way to make it available (possibly as an extra param from the caller, or possible as extra info on the RenderTreeFrame itself).

rynowak · 2018-04-09T02:07:48Z

src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs

+                // TLDR: If the component uses a method group or a non-capturing lambda
+                // we don't allocate much.
+               _eventHandlersById.Add(id, (UIEventArgs e) => @delegate.DynamicInvoke(e));
+            }


This was the trick. We only generate a 'wrapper' after we've processed the diff and decided to register the delegate for events. This allows the diff phase to leverage the delegate equality rules

Makes sense. Sounds like a good way to get the perf benefits.

rynowak · 2018-04-09T02:08:45Z

src/Microsoft.AspNetCore.Blazor/UIEventArgs.cs

    /// </summary>
-    public class UIChangeEventArgs : UIEventArgs
+    public class UIMouseEventArgs : UIEventArgs


Just sorted these since we're going to be adding more of them

rynowak · 2018-04-09T02:10:07Z

src/Microsoft.AspNetCore.Blazor/UIEventHandlerRenderTreeBuilderExtensions.cs

+        /// <param name="sequence">An integer that represents the position of the instruction in the source code.</param>
+        /// <param name="name">The name of the attribute.</param>
+        /// <param name="value">The value of the attribute.</param>
+        public static void AddAttribute(this RenderTreeBuilder builder, int sequence, string name, UIChangeEventHandler value)


This was the secret sauce to support method group conversion. Trying to use AddAttribute<T>(..., Action<T>) will cause overload resolution to fail.

rynowak · 2018-04-09T02:10:31Z

src/Microsoft.AspNetCore.Blazor/UIEventHandler.cs

+    /// <summary>
+    /// Handles an <see cref="UIMouseEventArgs"/> event raised for a <see cref="RenderTreeFrame"/>.
+    /// </summary>
+    public delegate void UIMouseEventHandler(UIMouseEventArgs e);


It seemed worth defining these as nominal types. Not strictly necessary though

rynowak · 2018-04-09T02:12:22Z

test/Microsoft.AspNetCore.Blazor.Build.Test/ComponentRenderingRazorIntegrationTest.cs

@@ -203,7 +203,7 @@ @using Microsoft.AspNetCore.Blazor
                    AssertFrame.Attribute(frame, "OnClick", 1);

                    // The handler will have been assigned to a lambda
-                    var handler = Assert.IsType<UIEventHandler>(frame.AttributeValue);
+                    var handler = Assert.IsType<UIMouseEventHandler>(frame.AttributeValue);


BTW the original version of this test still works. There was a lot of usage of UIEventArgs so I changed one to use a more specific type.

rynowak · 2018-04-09T02:12:36Z

test/Microsoft.AspNetCore.Blazor.Build.Test/RenderingRazorIntegrationTest.cs

@@ -552,7 +552,7 @@ @using Microsoft.AspNetCore.Blazor
                {
                    AssertFrame.Attribute(frame, "onclick", 1);

-                    var func = Assert.IsType<UIEventHandler>(frame.AttributeValue);
+                    var func = Assert.IsType<Action<UIMouseEventArgs>>(frame.AttributeValue);


This is the detail we wanted to change 👍

rynowak · 2018-04-09T02:15:21Z

test/Microsoft.AspNetCore.Blazor.E2ETest/Infrastructure/BasicTestAppTestBase.cs

@@ -7,15 +7,19 @@
 using OpenQA.Selenium;
 using OpenQA.Selenium.Support.UI;
 using System;
+using Xunit.Abstractions;


OK. So the rest of this is plumbing to make all of our E2E tests record selenium's logging info (including errors from js).

It's gross, because outputting data in xUnit has lots of caveats, and adding cross cutting behavior to tests has lots of limitations. So this has lots of hacks.

I'm hoping that once this is in we can silence the console I/O of selenium in our CI builds - since we would still see the logging for a failed test. This would really improve the usability of travis and appveyor since they choke under the weight of our output.

Also the errors that selenium reports for exceptions thrown from wasm don't have call stacks 😢 even though they do in the developer tools console. This at least will tell you at a glance if your failing test if you had an unhandled exception and what the message was.

We might be able to do more in the js part of the runtime to improve this in the future.

SteveSandersonMS · 2018-04-09T12:42:49Z

From the commit message:

You will still get some inefficient behavior if your component uses a
capturing lambda in an event handler, so don't do that.

While I understand there's nothing we can do about the fact that you're creating a different delegate instance on each render, it should be noted that capturing lambdas are legitimate and necessary in many cases. For example, if you're rendering a list of items and want a "delete" button next to each one, you're probably going to use a capturing lambda for that. It is technically possible to avoid the capturing lambda by having a separate component instance for each row, but that's quite a bit more work for the developer in some cases, so the capturing lambda is still probably preferable there.

SteveSandersonMS · 2018-04-09T12:47:56Z

We probably should have discussed this earlier, but what would you consider to be a good way for us to deal with async event handlers? Previously we were going to have @onclick and @onclickAsync so that you could declare either void MyHandler() or async Task MyHandler().

Do you think we'll end up extending the syntax to onclick-async=@MyMethod, or is there some way that we could make it work at compile-time if MyMethod returned either void or Task?

I'm not suggesting we should block this PR for that, but it would be good to be sure we have a likely plan in mind.

Update: Have filed #519 to focus on this.

SteveSandersonMS

Looks like the end result here is going to be what we were hoping for - nice job!

I'd still like to clarify the exact scenarios where we end up doing the DynamicInvoke. Is it the case that compile-time resolved UIEventHandler-compatible lambdas avoid it, and it's only used if you pass some other delegate type (and then we rely on it being compatible at runtime)?

Finally I hope we can avoid the need to inspect the delegate parameter types at runtime just by not worrying if we assign event handler IDs to things that wouldn't use them (unless you have reason to believe this will be extremely common).

SteveSandersonMS · 2018-04-09T11:17:36Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeBuilder.cs

@@ -185,7 +223,7 @@ public void AddAttribute(int sequence, string name, object value)

                    // Don't add anything for false bool value.
                }
-                else if (value is UIEventHandler eventHandler)
+                else if (value is MulticastDelegate eventHandler)


Not a big deal, but the variable eventHandler isn't used (nor was it prior to this commit).

SteveSandersonMS · 2018-04-09T12:15:06Z

src/Microsoft.AspNetCore.Blazor/UIEventHandlerRenderTreeBuilderExtensions.cs

+
+namespace Microsoft.AspNetCore.Blazor
+{
+    public static class UIEventHandlerRenderTreeBuilderExtensions


It would be great to include a comment block at the top of this file explaining the scenarios where these overloads are needed. I was finding it hard to figure out without actually commenting them out to see what broke, given that there's also an overload that takes UIEventHandler (the base type) directly and has exactly the same logic inside it.

In fact I think it would be better still if it could document the overall flow, in terms of how different Razor syntaxes lead to different AddAttribute calls, what the different method overload resolution possibilities are, and what this means for the different code paths at runtime. It will be tricky to keep track of all this otherwise, as the implementation is spread out over quite a lot of files.

I know there always a risk that documentation like that tends to rot over time, but on this occasion it's hard to keep track of it at all otherwise. What do you think?

Adding a comment, but fundamentally they are needed for cases in C# like:

builder.AddAttribute(15, "onkeypress", MyKeyPressHandler);

In Razor, we know the type of TEventArgs, because it's part of the attribute mapping - so we don't need this for Razor cases.

Needed in C# because of these few facts:

UIKeyboardEventHandler is-not-a UIEventHandler even though UIKeyboardEventArgs is-a UIEventArgs

method group to delegate conversion will therefore not allow us to assign a UIKeyboardEventHandler to UIEventHandler (the opposite works)

If we try to define AddAttribute<TEventArgs>(..., Action<TEventArgs> value) where T : UIEventArgs to resolve this, then that will be less preferred than some other overloads

If we define AddAttribute(..., UIKeyboardEventHandler value) then that will cause havok with lambda type inference

So adding these as extension methods is a goldilocks solution. It doesn't effect lambda type inference (all lambdas in C# are UIEventHandler), but it provides candidates for the delegate conversion.

SteveSandersonMS · 2018-04-09T12:21:54Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffBuilder.cs

            {
-                diffContext.Renderer.AssignEventHandlerId(ref newFrame);
+                var parameters = @delegate.Method.GetParameters();


I think GetParameters allocates (i.e., for the returned array). I'd strongly value not allocating here on every render, even at the cost of less fidelity over which delegate types we assign event handler IDs to.

OK great. Sounds like we'll be able to avoid this.

SteveSandersonMS · 2018-04-09T12:24:20Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffBuilder.cs

+                    typeof(UIEventArgs).IsAssignableFrom(parameters[0].ParameterType))
+                {
+                    diffContext.Renderer.AssignEventHandlerId(ref newFrame);
+                }


I assume we only want to 'register' delegates that can receive dom events

I'm not sure that's so important. Are there many likely scenarios where someone tries to use onsomething=@myDelegate where myDelegate isn't a UIEventHandler? Even if someone does that and we assign an event handler ID that never gets used, that doesn't seem like a big problem.

I'd favor optimizing for the common case (where the delegate is a UIEventHandler) even if in the uncommon case we do something a little less inefficient.

SteveSandersonMS · 2018-04-09T12:36:29Z

src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs

+            // In order to dispatch the event, we need a UIEventHandler, so we're going weakly
+            // typed here. The user will get a cast exception if they map the wrong type of
+            // delegate to the event.
+            if (frame.AttributeValue is UIEventHandler wrapper)


I will go and read the code until I understand it properly, but at first glance I'm struggling to tell why it's sometimes a UIEventHandler and other times a MulticastDelegate. Might be useful to have a comment specifying that explicitly.

rynowak · 2018-04-09T15:02:16Z

it should be noted that capturing lambdas are legitimate and necessary in many cases. For example, if you're rendering a list of items and want a "delete" button next to each one, you're probably going to use a capturing lambda for that. It is technically possible to avoid the capturing lambda by having a separate component instance for each row, but that's quite a bit more work for the developer in some cases, so the capturing lambda is still probably preferable there.

The old school workaround for this would be to pass an additional state parameter. I don't want to do that though, I think this is good enough and we'll never really prevent folks from using capturing lambdas even if we try.

rynowak · 2018-04-09T15:05:21Z

Is it the case that compile-time resolved UIEventHandler-compatible lambdas avoid it, and it's only used if you pass some other delegate type (and then we rely on it being compatible at runtime)?

The resolution is runtime based, since it's relies on casts. And yes, if you pass us something like like a UIKeyboardEventHandler or Action<UIEventHandler> it will be checked at runtime by Mono.

Finally I hope we can avoid the need to inspect the delegate parameter types at runtime just by not worrying if we assign event handler IDs to things that wouldn't use them (unless you have reason to believe this will be extremely common).

That would streamline this, as well as make it possible to use things like Action<object>. I'll try this out.

@clicked

Improves support for for other types of event handlers with eventargs types derived from UIEventArgs. Additionally fleshes out the set of event handler types. This change improves support for using more specific event handler types like: ``` <button onclick="@clicked" /> @functions { public void Clicked(UIMouseEventArgs e) { ... } } ``` And: ``` builder.AddAttribute(12, "onkeypressed", KeyPressed); ... void KeyPressed(UIKeyboardEventArgs e) { ... } ``` In particular what got better is: - overload resolution for the AddAttribute method - performance of different cases for AddAttribute ----- The runtime now treats delegates as one of three types: - arbitrary delegate: not attached to DOM events, not tracked by renderer - UIEventHandler: can attach to DOM events, tracked by renderer, first class in IHandleEvents - UIEventHandler-like: can attach to DOM events, tracked by renderer, requires some special runtime support. The set of overloads on AddAttribute has been tuned with a few specific cases in mind. Lambda expressions in an attribute will be inferred as UIEventHandler unless the compiler does something more specific. So for instance, passing a lambda as an attribute value for a component, where the component doesn't define a matching attribute, will always be inferred as UIEventHandler. We now support method-group to delegate conversion for methods that accept a derived UIEventArgs type. This means you can use a signature like `void KeyPressed(UIKeyboardEventArgs e)` without any compiler magic, and this will work in the runtime as long as the event type produced by the runtime matches. We also allow user-defined UIEventArgs-derived types. There's a pattern for this and it requires defining an extension method and delegate type. The method-group to delegate conversion part required some doing. It doesn't play well with generics (Action<T> where T : UIEventArgs) doesn't work at all. Adding more actual overloads (as opposed to extensions) would cause lambda cases we want to work to be ambiguous. ---- The performance win here is to remove the need for a 'wrapper' delegate created by the event handler tag helper code. This wrapper is now created by the runtime, but only *after* we have checked the frame for changes. This requires more heavy lifting in the runtime, but has the advantage of producing no-op diffs as often as possible. You will still get some inefficient behavior if your component uses a capturing lambda in an event handler, so don't do that.

rynowak · 2018-04-09T20:12:26Z

@SteveSandersonMS - I think the changes to this worked out well.

We no longer look at the type of the delegate and instead register it if it matches the "onfoo" naming pattern. This means you can use types like Action<object> and it will still work just fine

@clicked

* Improve support for more types of event handlers Improves support for for other types of event handlers with eventargs types derived from UIEventArgs. Additionally fleshes out the set of event handler types. This change improves support for using more specific event handler types like: ``` <button onclick="@clicked" /> @functions { public void Clicked(UIMouseEventArgs e) { ... } } ``` And: ``` builder.AddAttribute(12, "onkeypressed", KeyPressed); ... void KeyPressed(UIKeyboardEventArgs e) { ... } ``` In particular what got better is: - overload resolution for the AddAttribute method - performance of different cases for AddAttribute ----- The runtime now treats delegates as one of three types: - arbitrary delegate: not attached to DOM events, not tracked by renderer - UIEventHandler: can attach to DOM events, tracked by renderer, first class in IHandleEvents - UIEventHandler-like: can attach to DOM events, tracked by renderer, requires some special runtime support. The set of overloads on AddAttribute has been tuned with a few specific cases in mind. Lambda expressions in an attribute will be inferred as UIEventHandler unless the compiler does something more specific. So for instance, passing a lambda as an attribute value for a component, where the component doesn't define a matching attribute, will always be inferred as UIEventHandler. We now support method-group to delegate conversion for methods that accept a derived UIEventArgs type. This means you can use a signature like `void KeyPressed(UIKeyboardEventArgs e)` without any compiler magic, and this will work in the runtime as long as the event type produced by the runtime matches. We also allow user-defined UIEventArgs-derived types. There's a pattern for this and it requires defining an extension method and delegate type. The method-group to delegate conversion part required some doing. It doesn't play well with generics (Action<T> where T : UIEventArgs) doesn't work at all. Adding more actual overloads (as opposed to extensions) would cause lambda cases we want to work to be ambiguous. ---- The performance win here is to remove the need for a 'wrapper' delegate created by the event handler tag helper code. This wrapper is now created by the runtime, but only *after* we have checked the frame for changes. This requires more heavy lifting in the runtime, but has the advantage of producing no-op diffs as often as possible. You will still get some inefficient behavior if your component uses a capturing lambda in an event handler, so don't do that. * Add selenium logs to test output * Minor feedback * WIP

rynowak requested a review from SteveSandersonMS April 9, 2018 01:56

rynowak commented Apr 9, 2018

View reviewed changes

rynowak force-pushed the rynowak/delegate-types branch from 54fc2de to 9828ddb Compare April 9, 2018 02:08

rynowak commented Apr 9, 2018

View reviewed changes

SteveSandersonMS approved these changes Apr 9, 2018

View reviewed changes

SteveSandersonMS mentioned this pull request Apr 9, 2018

Improve JS-side event handling code #521

Merged

rynowak added 3 commits April 9, 2018 08:06

Add selenium logs to test output

143d3df

Minor feedback

b2b0e0e

rynowak force-pushed the rynowak/delegate-types branch from 9828ddb to b2b0e0e Compare April 9, 2018 18:13

WIP

189e83d

rynowak merged commit a20e6c2 into dev Apr 9, 2018

rynowak deleted the rynowak/delegate-types branch April 9, 2018 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements for delegate types #516

Improvements for delegate types #516

rynowak commented Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

rynowak Apr 9, 2018 •

edited

rynowak Apr 9, 2018

rynowak Apr 9, 2018

rynowak Apr 9, 2018

rynowak Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS commented Apr 9, 2018

SteveSandersonMS commented Apr 9, 2018 •

edited

SteveSandersonMS left a comment

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS Apr 9, 2018

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

rynowak Apr 9, 2018 •

edited

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

SteveSandersonMS Apr 9, 2018

SteveSandersonMS Apr 9, 2018

rynowak Apr 9, 2018

rynowak commented Apr 9, 2018 •

edited

rynowak commented Apr 9, 2018

rynowak commented Apr 9, 2018 •

edited

Improvements for delegate types #516

Improvements for delegate types #516

Conversation

rynowak commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak Apr 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Apr 9, 2018

SteveSandersonMS commented Apr 9, 2018 • edited

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak Apr 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Apr 9, 2018 • edited

rynowak commented Apr 9, 2018

rynowak commented Apr 9, 2018 • edited

rynowak Apr 9, 2018 •

edited

SteveSandersonMS commented Apr 9, 2018 •

edited

rynowak Apr 9, 2018 •

edited

rynowak commented Apr 9, 2018 •

edited

rynowak commented Apr 9, 2018 •

edited